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

Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

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.

- 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.
Expand Down
30 changes: 26 additions & 4 deletions src/runtime/assemblymanager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,17 @@ public static Assembly LoadAssembly(string name)
Assembly assembly = null;
try
{
assembly = Assembly.Load(name);
var importEvent = new ImplicitAssemblyLoadingEventArgs(name);
Copy link
Contributor

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.

if (importEvent.SkipAssemblyLoad)
{
return null;
}

PythonEngine.RaiseAssemblyAsModuleImportingEvent(importEvent);
if (!importEvent.SkipAssemblyLoad)
{
assembly = Assembly.Load(name);
}
}
catch (Exception)
{
Expand Down Expand Up @@ -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();
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 Expand Up @@ -419,12 +438,15 @@ public static List<string> GetNames(string nsname)
{
foreach (Assembly a in namespaces[nsname].Keys)
{
Type[] types = a.GetTypes();
Type[] types = a.IsDynamic ? a.GetTypes(): a.GetExportedTypes();
foreach (Type t in types)
{
if ((t.Namespace ?? "") == nsname)
{
names.Add(t.Name);
if (!t.IsNested)
Copy link
Contributor

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?

Copy link
Contributor

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.

{
names.Add(t.Name);
}
}
}
}
Expand Down
30 changes: 29 additions & 1 deletion src/runtime/pythonengine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
Expand Down Expand Up @@ -130,6 +130,11 @@ public static string Compiler
get { return Marshal.PtrToStringAnsi(Runtime.Py_GetCompiler()); }
}

/// <summary>
/// Fires when python engines importing module and probably tries to load an assembly.
/// </summary>
public static event EventHandler<ImplicitAssemblyLoadingEventArgs> ImplicitAssemblyLoading;

public static int RunSimpleString(string code)
{
return Runtime.PyRun_SimpleString(code);
Expand Down Expand Up @@ -520,6 +525,29 @@ internal static PyObject RunString(string code, IntPtr? globals, IntPtr? locals,
}
}
}

internal static void RaiseAssemblyAsModuleImportingEvent(ImplicitAssemblyLoadingEventArgs e)
{
ImplicitAssemblyLoading?.Invoke(null, e);
}
}

public class ImplicitAssemblyLoadingEventArgs: EventArgs
{
public ImplicitAssemblyLoadingEventArgs(string moduleName)
{
ModuleName = moduleName;
}

/// <summary>
/// The name of the module to import that is probably assembly name.
/// </summary>
public string ModuleName { get; }

/// <summary>
/// Set it to true, if you know that <see cref="ModuleName"/> is not an assembly to import.
/// </summary>
public bool SkipAssemblyLoad { get; set; }
}

public enum RunFlagType
Expand Down