Feature: Lazy Loading of Transitive Shared Types [See Comments] #164

Sewer56 · 2020-10-11T10:22:10Z

Context

Please see Issue #163 for details.
This PR is also the same as #158 , except that I wanted to rename the source branch and split into Issue + PR.

Solution

This PR adds an optional feature IsLazyLoaded in PluginConfig.
Relevant patches are made in AssemblyLoadContextBuilder.PreferDefaultLoadContextAssembly and ManagedLoadContext.Load.

Risks

The intended/main usage of the library, host sharing a contract in the form of a known interface at compile time with the plugins should be unaffected by this optional feature. There should not be any functional difference introduced by not loading ahead of time.

Transitive dependencies may come to mind, specifically dependencies of the plugin that the host is also aware or has a copy of. These function just the same way as if they were loaded ahead of time; I added a test for this just in case.

I have not managed to identify any cases in which the introduction of this feature would break existing functionality.

Tests

As for the test project; I wasn't really sure if this feature needed any special specific tests outside of ensuring transitive assemblies are still correctly resolved; so I added an additional variation of TransitiveAssembliesOfSharedTypesAreResolved in SharedTypesTests that uses the new setting to ensure correct resolution.

I also tested this on real software and a few random existing tests just in case, everything seems to be working properly.

Other Miscellaneous Changes (Performance)

PluginLoader.CreateFromAssemblyFile(string, Type[], Action<PluginConfig>)

Instead of adding the assembly for each type, I considered that there are end users (unfamiliar to inner workings) who may pass multiple types belonging to the same assembly to the method. With that in mind, I made the library remove duplicates before they are added to config.SharedAssemblies.

The main reason for this is down the road, the loader will call AssemblyLoadContextBuilder.PreferDefaultLoadContextAssembly; this is an expensive operation when lazy loading is not enabled and it does not need to be executed multiple times with the same assembly name.

Sewer56 · 2020-10-11T10:23:42Z

Real World Performance Test

People tend to often showcase the best case scenario for changes; I figured I'd go the other way around and show the worst case scenario instead.

Notably; we are working around an I/O bottleneck here so the worst case condition is to perform a hot load (i.e. plugins are either cached by the OS in memory or by hardware). This was performed by loading 5 times in a row and taking the best time before and after.

Before:

After:

This is Reloaded II, using a sample of 16 plugins.

Specifically benchmarked is the process described in #60 (comment) , where Loading Assembly Metadata refers to step 2 (for all plugins); and initialising refers to step 3; both performed in parallel to maximise performance (this is an I/O bottleneck after all).

Real world performance improvement for "worst case" tends to be in the range of 2.5x-3.0x considering the numbers also include finding the entry point etc. I haven't tested cold loads (best case scenario as this is an I/O bottleneck) but I'd expect the difference to be more significant.

natemcmaster

Thanks for posting this proposal, @Sewer56. I have a few questions before we proceed

natemcmaster · 2020-12-27T20:10:25Z

src/Plugins/Loader/AssemblyLoadContextBuilder.cs

+            var names = new Queue<AssemblyName>();
+            names.Enqueue(assemblyName);
+            while (names.TryDequeue(out var name))
+            {
+                if (name.Name == null || _defaultAssemblies.Contains(name.Name))
+                {
+                    // base cases
+                    continue;
+                }

-            _defaultAssemblies.Add(assemblyName.Name);
+                _defaultAssemblies.Add(name.Name);

-            // Recursively load and find all dependencies of default assemblies.
-            // This sacrifices some performance for determinism in how transitive
-            // dependencies will be shared between host and plugin.
-            var assembly = _defaultLoadContext.LoadFromAssemblyName(assemblyName);
-            foreach (var reference in assembly.GetReferencedAssemblies())
-            {
-                PreferDefaultLoadContextAssembly(reference);
+                // Load and find all dependencies of default assemblies.
+                // This sacrifices some performance for determinism in how transitive
+                // dependencies will be shared between host and plugin.
+                var assembly = _defaultLoadContext.LoadFromAssemblyName(name);
+
+                foreach (var reference in assembly.GetReferencedAssemblies())
+                {
+                    names.Enqueue(reference);
+                }
            }


What is the purpose of this refactor?

I did it because the original method was recursive.

I wasn't really sure whether you would have preferred to (from a style perspective) to have a separate internal method for the non-lazy implementation. Personally, I just thought the idea of checking a branch that would always have been false in the beginning seemed a bit silly to me.

Doing it this way is also technically faster due to less branches/jumps; albeit by such a negligible non-perceptible amount it's not worth caring about.

src/Plugins/Loader/AssemblyLoadContextBuilder.cs

natemcmaster · 2020-12-27T20:17:07Z

src/Plugins/Loader/ManagedLoadContext.cs

@@ -40,11 +41,12 @@ internal class ManagedLoadContext : AssemblyLoadContext
            IReadOnlyDictionary<string, ManagedLibrary> managedAssemblies,
            IReadOnlyDictionary<string, NativeLibrary> nativeLibraries,
            IReadOnlyCollection<string> privateAssemblies,
-            IReadOnlyCollection<string> defaultAssemblies,
+            ICollection<string> defaultAssemblies,


Can we find a way to code this without changing this? The intention behind read-only behaviors was to minimize the chance for unintended side-effects between contexts.

Are you referring to the parameter specifically or the class field?
I suppose one option is to have read only parameter but copy to our own private collection in the constructor.

This way the API would still signal to the user that the collection is immutable/cannot be changed once instantiated.

natemcmaster · 2020-12-27T20:21:40Z

test/Plugins.Tests/SharedTypesTests.cs

+        /// <summary>
+        /// Variant of <see cref="TransitiveAssembliesOfSharedTypesAreResolved"/> which uses lazy loading for assemblies.
+        /// </summary>
+        [Fact]


Instead of two separate test methods, this could be simplified by just making IsLazyLoaded a parameter.

[Theory] [InlineData(true)] [InlineData(false)] public void TransitiveAssembliesOfSharedTypesAreResolved(bool isLazyLoaded)

Completely agree here; not really sure what I was thinking.
Maybe I thought that it would be easier to look at when checking reports.
That was silly of me; I'll fix that.

natemcmaster · 2020-12-27T20:28:29Z

Besides my questions about the actual changes to code, my main question here is about the tradeoff between performance and possible non-determinism in loading dependencies. In your perf tests above, it shows this can save 20-30 milliseconds on loading a plugin. Is that amount of time worth it if this leads to strange, hard to reproduce bugs caused by dependencies loading in an unexpected way?

Sewer56 · 2020-12-28T03:04:47Z

Besides my questions about the actual changes to code, my main question here is about the tradeoff between performance and possible non-determinism in loading dependencies. In your perf tests above, it shows this can save 20-30 milliseconds on loading a plugin. Is that amount of time worth it if this leads to strange, hard to reproduce bugs caused by dependencies loading in an unexpected way?

In retrospect; it's something that should probably have a clear warning sticker put on it.
There is one single failure case; which albeit very unlikely to happen in practice should be documented.

More specifically; what happens if the plugin references a library which is also referenced by a transitive shared assembly (TransitiveTransitiveSharedAssembly); and loads/uses it before the transitive shared type is loaded (unification fails because TransitiveTransitiveSharedAssembly isn't in defaultAssemblies).

I made a branch to demonstrate this. A test should fail as expected.

I should note that transitively shared assemblies in general are rather dangerous and is a practice I'd recommend avoiding if possible. If there are two plugins expecting a different version of the same transitively shared assembly with incompatible APIs, all hell can break loose.

Anyway, that said, I still think there is use for this feature however; for example those situations where you do only need to extract a small amount of data, such as metadata about the author, name etc. of the plugin before properly loading it in the future. In my own case, I have a setup where plugins can share types between each other using the main program as a proxy, and thus identifying exported types faster helps me (see: Loading Assembly Metadata in screenshots above). It also helps reduce stutters in a WPF application where I query for a certain interface to see if a plugin is configurable.

In practice the difference isn't too huge but I'm personally the kind of person where if I can optimize something for a noticeably better user experience, even if minor, I'd do it.

Sewer56 · 2020-12-28T03:29:09Z

I made minor changes to this PR based on the feedback; please let me know if those are a-ok with you.

natemcmaster · 2021-01-30T22:41:03Z

@Sewer56 thanks for the reply and detailed description! Thanks for your patience on this one. Let me re-review this code and get back to you soon about merging this.

codecov · 2021-01-31T01:18:40Z

Codecov Report

Merging #164 (a050596) into main (5cd23e7) will increase coverage by 0.88%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
+ Coverage   81.86%   82.75%   +0.88%     
==========================================
  Files          15       15              
  Lines         546      574      +28     
==========================================
+ Hits          447      475      +28     
  Misses         99       99

Impacted Files	Coverage Δ
src/Plugins/Loader/AssemblyLoadContextBuilder.cs	`82.52% <100.00%> (+2.74%)`	⬆️
src/Plugins/Loader/ManagedLoadContext.cs	`78.80% <100.00%> (+0.87%)`	⬆️
src/Plugins/PluginConfig.cs	`84.21% <100.00%> (+0.87%)`	⬆️
src/Plugins/PluginLoader.cs	`81.25% <100.00%> (+1.25%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cd23e7...a050596. Read the comment docs.

Sewer56 added 3 commits September 12, 2020 13:51

PluginLoader: Add only unique assemblies when provided types.

dfaad9a

ContextBuilder: Replace recursion with iteration & heap based stack.

527ff42

Feature: Optional Lazy Loading for Dependencies

ae7699d

Sewer56 requested a review from natemcmaster as a code owner October 11, 2020 10:22

Sewer56 changed the title ~~Feature: Lazy Loading of Shared Types~~ Feature: Lazy Loading of Transitive Shared Types Oct 11, 2020

Sewer56 mentioned this pull request Oct 11, 2020

[Feature Request] Lazy Loading #163

Closed

Sewer56 changed the title ~~Feature: Lazy Loading of Transitive Shared Types~~ Feature: Lazy Loading of Transitive Shared Types [See Comments] Oct 12, 2020

natemcmaster reviewed Dec 27, 2020

View reviewed changes

natemcmaster linked an issue Dec 27, 2020 that may be closed by this pull request

[Feature Request] Lazy Loading #163

Closed

Minor adjustments based on PR feedback

9fd191c

Sewer56 force-pushed the lazyloading branch from 39ff33e to 9fd191c Compare December 28, 2020 03:21

natemcmaster added 3 commits January 30, 2021 17:05

Merge branch 'main' into pr/Sewer56/164

dff78fd

Run dotnet format

64f8972

Update release notes

a867df1

natemcmaster added this to the 1.4.0 milestone Jan 31, 2021

natemcmaster added the enhancement New feature or request label Jan 31, 2021

natemcmaster added 2 commits January 30, 2021 17:15

Fix bad merge from main branch

7b43eb3

fix line endings

a050596

natemcmaster approved these changes Jan 31, 2021

View reviewed changes

natemcmaster merged commit 6f40206 into natemcmaster:main Jan 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Lazy Loading of Transitive Shared Types [See Comments] #164

Feature: Lazy Loading of Transitive Shared Types [See Comments] #164

Sewer56 commented Oct 11, 2020 •

edited

Loading

Sewer56 commented Oct 11, 2020

natemcmaster left a comment

natemcmaster Dec 27, 2020

Sewer56 Dec 27, 2020

natemcmaster Dec 27, 2020

Sewer56 Dec 28, 2020

natemcmaster Dec 27, 2020

Sewer56 Dec 28, 2020

natemcmaster commented Dec 27, 2020

Sewer56 commented Dec 28, 2020

Sewer56 commented Dec 28, 2020

natemcmaster commented Jan 30, 2021

codecov bot commented Jan 31, 2021 •

edited

Loading

Feature: Lazy Loading of Transitive Shared Types [See Comments] #164

Feature: Lazy Loading of Transitive Shared Types [See Comments] #164

Conversation

Sewer56 commented Oct 11, 2020 • edited Loading

Context

Solution

Risks

Tests

Other Miscellaneous Changes (Performance)

Sewer56 commented Oct 11, 2020

Real World Performance Test

natemcmaster left a comment

Choose a reason for hiding this comment

natemcmaster Dec 27, 2020

Choose a reason for hiding this comment

Sewer56 Dec 27, 2020

Choose a reason for hiding this comment

natemcmaster Dec 27, 2020

Choose a reason for hiding this comment

Sewer56 Dec 28, 2020

Choose a reason for hiding this comment

natemcmaster Dec 27, 2020

Choose a reason for hiding this comment

Sewer56 Dec 28, 2020

Choose a reason for hiding this comment

natemcmaster commented Dec 27, 2020

Sewer56 commented Dec 28, 2020

Sewer56 commented Dec 28, 2020

natemcmaster commented Jan 30, 2021

codecov bot commented Jan 31, 2021 • edited Loading

Codecov Report

Sewer56 commented Oct 11, 2020 •

edited

Loading

codecov bot commented Jan 31, 2021 •

edited

Loading