- Notifications
You must be signed in to change notification settings - Fork750
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.
Conversation
mention-bot commentedAug 23, 2017
@dmitriyse, thanks!@vmuriart,@tonyroberts,@cgohlke,@tiran and@BartonCline, please review this. |
codecovbot commentedAug 23, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
@@ Coverage Diff @@## master #528 +/- ##==========================================- Coverage 77.4% 76.76% -0.64%========================================== Files 63 64 +1 Lines 5589 5630 +41 Branches 892 894 +2 ==========================================- Hits 4326 4322 -4- Misses 970 1013 +43- Partials 293 295 +2
Continue to review full report at Codecov.
|
a13e4f6 toe2efddaComparee2efdda to9304d72Compare09acf53 toe7edeabComparee7edeab to1fd5998Compare1fd5998 toe4764f2Comparee4764f2 tod09fc92Compare| 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 comment
The reason will be displayed to describe this comment to others.Learn more.
@dmitriyse can you please add explanation for why you added check forisDynamic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 comment
The 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 comment
The 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 comment
The 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.
Type.IsNested types are excluded from the names collection that is used for implicit assembly loading.
| foreach (Type t in types) | ||
| { | ||
| if ((t.Namespace ?? "") == nsname) | ||
| { | ||
| names.Add(t.Name); | ||
| if (!t.IsNested) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 comment
The 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.
| 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 comment
The reason will be displayed to describe this comment to others.Learn more.
@dmitriyse what do you mean by transitive dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 comment
The 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
https://msdn.microsoft.com/en-us/library/system.reflection.reflectiontypeloadexception(v=vs.110).aspx
| @@ -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 comment
The 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.
| @@ -196,7 +196,17 @@ public static Assembly LoadAssembly(string name) | |||
| Assembly assembly = null; | |||
| try | |||
| { | |||
| assembly = Assembly.Load(name); | |||
| var importEvent = new ImplicitAssemblyLoadingEventArgs(name); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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.
@dmitriyse I left some comments in this pull request. Once you explain the intent of this code, feel free to merge this one with all CI's passing tests. And please address@Cronan concern with likely incorrect exception names. |
@Cronan thanks for reviewing this! @dmitriyse if you can come up with a test for nested types, that would be great. |
To be honest, I don't see the point of the whole event/event-args part of this pull request. |
Superceded by#723. |
What does this implement/fix? Explain your changes.
Does this close any currently open issues?
no
...
Any other comments?
VS 2017 + Docker is insensitive to DebuggerNonUserCode, and LoadAssembly exceptions produces long trash in the debug log.
...
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG