Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Optimizes implicit assembly loading. Helps to reduce amount of faulted LoadAssembly calls. #528

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8ee0c65
Implicit assembly loading optimization.
Aug 23, 2017
d09fc92
Assembly loading experience improved + fixes.
Sep 7, 2017
4af13a3
Merge branch 'master' into implicit_assembly_load_opt
den-run-ai Feb 10, 2018
c28bafd
Merge branch 'master' into implicit_assembly_load_opt
den-run-ai Feb 10, 2018
f016059
Merge branch 'master' into implicit_assembly_load_opt
filmor Aug 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Assembly loading experience improved + fixes.
  • Loading branch information
dse committed Sep 7, 2017
commit d09fc92c45a06911668e655bb9a77b002cace5ad
17 changes: 14 additions & 3 deletions src/runtime/assemblymanager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,10 @@ public static Assembly LoadAssembly(string name)
}

PythonEngine.RaiseAssemblyAsModuleImportingEvent(importEvent);

assembly = Assembly.Load(name);
if (!importEvent.SkipAssemblyLoad)
{
assembly = Assembly.Load(name);
}
}
catch (Exception)
{
Expand Down Expand Up @@ -351,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();
Copy link
Contributor

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 for isDynamic?

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the documentation, GetExportedTypes() is not supported on dynamic types and will throw NotSupportedException

Copy link
Contributor

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.

Copy link
Contributor

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.

}
catch(TypeLoadException)
{
// Do nothing.
// This problem usually occurs when transitive dependencies have references to older packages than main application.
Copy link
Contributor

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?

Copy link
Contributor

@Cronan Cronan Feb 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be wrong, but ReflectionTypeLoadException is thrown by GetTypes() if any of the dependent assemblies can't be loaded (with a list of the loaded types), but GetExportedTypes() throws FileNotFoundException with no information. I'm not sure if TypeLoadException is thrown here.

Copy link
Contributor

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

}

Type[] types = assembly.GetTypes();
foreach (Type t in types)
{
string ns = t.Namespace ?? "";
Expand Down