opentelemetry-dotnet-instrumentation icon indicating copy to clipboard operation
opentelemetry-dotnet-instrumentation copied to clipboard

Loader optimizations

Open RassK opened this issue 2 years ago • 2 comments

Like we agreed in the SIG meeting I'm creating this issue to track and choose optimizations we want to do later for loading managed layer. I'll list them all here, feel free to add any points, comments etc >

  • Unite Managed.Loader and StartupHook. Their logic is like 99% similar, I guess even static entrypoints could be the same. ** This would reduce some assembly loading and reflection (win: all platforms)

  • Remove Managed.Loader from CMake build (CMakeLists.txt). StartupHook env vars can be still set manually. cor_profiler.cpp needs additional branching not to load Managed.Loader in case of Linux / MacOS. ** Optimizes build for Linux and MacOS (Windows is using MSBuild). ** Helps to remove CMake downgrade scripts and simplify CI.

  • Cleanup native layer. cor_profiler.cpp and etc could remove Linux / MacOS preprocessors that are dealing with loading the Managed.Loader. ** Code cleanup ** Optimizes the dll / deployment size

  • Since Managed.Loader is only required for .NET FX build, there are 2 ways to optimize the .NET core path.

    • Rearrange loading the assembly from separate dll (loading is done in cor_profiler.cpp CorProfiler::GetAssemblyAndSymbolsBytes)
    • Split native dll into 2 parts. Move Managed.Loader loading into second part, embed Managed.Loader as a resource to the second part. Make sure the 2nd part only executes in case of .NET FX.
  • [Needs testing] Let ICorProfiler to set env vars for StartupHook. ** Simpler out of the box experience, as less paths to set. ** Allows automatically to select most optimized build. For eg in case we want to use specialized types from the newer version of .NET and ship more performance optimized version of the package.

RassK avatar Oct 14 '21 09:10 RassK

Unite Managed.Loader and StartupHook. Their logic is like 99% similar, I guess even static entrypoints could be the same. ** This would reduce some assembly loading and reflection (win: all platforms)

This is a very good callout. Want to give more clarity on StartupHook. Currently, it is a wrapper that just brings the Otel CLR loader. All other assembly resolving features are removed from the StartupHook.

rajkumar-rangaraj avatar Oct 15 '21 16:10 rajkumar-rangaraj

Related issue https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/issues/323

pellared avatar Feb 23 '22 18:02 pellared

  • Unite Managed.Loader and StartupHook. Their logic is like 99% similar, I guess even static entrypoints could be the same. ** This would reduce some assembly loading and reflection (win: all platforms)

This was already done: StartupHook actually instantiate the Loader which takes care of initializing OpenTelemetry.AutoInstrumentation.

  • Remove Managed.Loader from CMake build (CMakeLists.txt). StartupHook env vars can be still set manually. cor_profiler.cpp needs additional branching not to load Managed.Loader in case of Linux / MacOS. ** Optimizes build for Linux and MacOS (Windows is using MSBuild). ** Helps to remove CMake downgrade scripts and simplify CI.

Removal from CMake being addressed via #2216 - Environment variables should still be separate to allow use of StartupHook in scenarios that is not possible to add the CLR profiler.

  • Cleanup native layer. cor_profiler.cpp and etc could remove Linux / MacOS preprocessors that are dealing with loading the Managed.Loader. ** Code cleanup ** Optimizes the dll / deployment size

Addressed via #2216

  • Since Managed.Loader is only required for .NET FX build, there are 2 ways to optimize the .NET core path.
    • Rearrange loading the assembly from separate dll (loading is done in cor_profiler.cpp CorProfiler::GetAssemblyAndSymbolsBytes)
    • Split native dll into 2 parts. Move Managed.Loader loading into second part, embed Managed.Loader as a resource to the second part. Make sure the 2nd part only executes in case of .NET FX.

Part of #2216: keeping it as a single DLL for simplicity, since a complete installation for Windows is going to take the disk space anyway. The optimization is in avoiding the JITCompilationStarted callback on .NET.

  • [Needs testing] Let ICorProfiler to set env vars for StartupHook. ** Simpler out of the box experience, as less paths to set. ** Allows automatically to select most optimized build. For eg in case we want to use specialized types from the newer version of .NET and ship more performance optimized version of the package.

This is possible, however, there are cases in which is not possible/acceptable to require the CLR Profiler so keeping the StartupHook configuration independent makes sense.

After #2216 is merged I don't see a reason to keep this issue open.

pjanotti avatar Feb 15 '23 15:02 pjanotti

Closing per comment above. A related possible change that was discussed was to move the StartupHook code to the same project as the loader, but, the benefits seem minimal.

pjanotti avatar Feb 16 '23 17:02 pjanotti