- Notifications
You must be signed in to change notification settings - Fork752
Optimizes implicit assembly loading. Helps to reduce amount of faulted LoadAssembly calls.#528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
8ee0c65d09fc924af13a3c28bafdf016059File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -8,6 +8,7 @@ This document follows the conventions laid out in [Keep a CHANGELOG][]. | ||
| ## [unreleased][] | ||
| ### Added | ||
| - Optimized implicit assembly loading on module import, PythonEngine.ImplicitAssemblyLoading event added. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @dmitriyse please add reference to this pull request and issues from which you referred to this pull request. | ||
| - Added support for embedding python into dotnet core 2.0 (NetStandard 2.0) | ||
| - Added new build system (pythonnet.15.sln) based on dotnetcore-sdk/xplat(crossplatform msbuild). | ||
| Currently there two side-by-side build systems that produces the same output (net40) from the same sources. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -196,7 +196,17 @@ public static Assembly LoadAssembly(string name) | ||
| Assembly assembly = null; | ||
| try | ||
| { | ||
| var importEvent = new ImplicitAssemblyLoadingEventArgs(name); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @dmitriyse why did you augment assembly loading with this event, I cannot see the purpose of this. | ||
| if (importEvent.SkipAssemblyLoad) | ||
| { | ||
| return null; | ||
| } | ||
| PythonEngine.RaiseAssemblyAsModuleImportingEvent(importEvent); | ||
| if (!importEvent.SkipAssemblyLoad) | ||
| { | ||
| assembly = Assembly.Load(name); | ||
| } | ||
| } | ||
| catch (Exception) | ||
| { | ||
| @@ -343,8 +353,17 @@ internal static void ScanAssembly(Assembly assembly) | ||
| // A couple of things we want to do here: first, we want to | ||
| // gather a list of all of the namespaces contributed to by | ||
| // the assembly. | ||
| Type[] types = new Type[0]; | ||
| try | ||
| { | ||
| types = assembly.IsDynamic ? assembly.GetTypes():assembly.GetExportedTypes(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @dmitriyse can you please add explanation for why you added check for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. should there be a test for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. According to thedocumentation, GetExportedTypes() is not supported on dynamic types and will throwNotSupportedException There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @Cronan good explanation! still maybe worth adding a comment about this. especially explaining the switch from GetTypes to GetExportedTypes for normal types. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @dmitriyse@Cronan ok, I see this comment in the header of the PR: Assembly.GetTypes replaced to Assembly.GetExportedTypes. So we are really skipping non-public types now. | ||
| } | ||
| catch(TypeLoadException) | ||
| { | ||
| // Do nothing. | ||
| // This problem usually occurs when transitive dependencies have references to older packages than main application. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @dmitriyse what do you mean by transitive dependencies? Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I may be wrong, butReflectionTypeLoadException is thrown by GetTypes() if any of the dependent assemblies can't be loaded (with a list of the loaded types), but GetExportedTypes() throwsFileNotFoundException with no information. I'm not sure if TypeLoadException is thrown here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @Cronan i think you are right about TypeLoadException -> ReflectionTypeLoadException, these are 2 different exceptions and I believe this was a typo from@dmitriyse: https://msdn.microsoft.com/en-us/library/system.typeloadexception(v=vs.110).aspx | ||
| } | ||
| foreach (Type t in types) | ||
| { | ||
| string ns = t.Namespace ?? ""; | ||
| @@ -419,12 +438,15 @@ public static List<string> GetNames(string nsname) | ||
| { | ||
| foreach (Assembly a in namespaces[nsname].Keys) | ||
| { | ||
| Type[] types = a.IsDynamic ? a.GetTypes(): a.GetExportedTypes(); | ||
| foreach (Type t in types) | ||
| { | ||
| if ((t.Namespace ?? "") == nsname) | ||
| { | ||
| if (!t.IsNested) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @why is this nested check needed? should there be a test for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. @dmitriyse ok, I see this comment in the header of the PR: Type.IsNested types are excluded from the names collection that is used for implicit assembly loading. | ||
| { | ||
| names.Add(t.Name); | ||
| } | ||
| } | ||
| } | ||
| } | ||