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

dmitriyse · 2017-08-23T19:03:36Z

What does this implement/fix? Explain your changes.

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.

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.

This fix speedups unit tests (up to 3x). And probably speedups initialization of production projects.
...

Checklist

Check all those that are applicable and complete.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Add yourself to AUTHORS
Updated the CHANGELOG

mention-bot · 2017-08-23T19:03:39Z

@dmitriyse, thanks! @vmuriart, @tonyroberts, @cgohlke, @tiran and @BartonCline, please review this.

codecov · 2017-08-23T19:51:15Z

Codecov Report

Merging #528 into master will decrease coverage by 0.63%.
The diff coverage is 61.9%.

@@            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

Flag	Coverage Δ
#setup_linux	`69.42% <ø> (ø)`	⬆️
#setup_windows	`75.95% <61.9%> (-0.63%)`	⬇️

Impacted Files	Coverage Δ
src/runtime/pythonengine.cs	`77.68% <57.14%> (-0.64%)`	⬇️
src/runtime/assemblymanager.cs	`87.3% <64.28%> (-1.8%)`	⬇️
src/runtime/constructorbinding.cs	`9.85% <0%> (-29.58%)`	⬇️
src/runtime/overload.cs	`58.33% <0%> (-15.58%)`	⬇️
src/runtime/pyiter.cs	`63.63% <0%> (-13.64%)`	⬇️
src/runtime/iterator.cs	`81.81% <0%> (-7.08%)`	⬇️
src/runtime/genericutil.cs	`82.81% <0%> (-3.13%)`	⬇️
src/runtime/moduleobject.cs	`81.6% <0%> (-1.54%)`	⬇️
src/runtime/pyobject.cs	`38.85% <0%> (-0.92%)`	⬇️
src/runtime/runtime.cs	`90.98% <0%> (-0.4%)`	⬇️
... and 5 more

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 fb84cd2...f016059. Read the comment docs.

den-run-ai · 2018-02-08T03:44:50Z

src/runtime/assemblymanager.cs

+            Type[] types = new Type[0];
+            try
+            {
+                types = assembly.IsDynamic ? assembly.GetTypes():assembly.GetExportedTypes();


@dmitriyse can you please add explanation for why you added check for isDynamic?

should there be a test for this?

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

@Cronan good explanation! still maybe worth adding a comment about this. especially explaining the switch from GetTypes to GetExportedTypes for normal types.

@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.

den-run-ai · 2018-02-08T03:45:37Z

src/runtime/assemblymanager.cs

                    foreach (Type t in types)
                    {
                        if ((t.Namespace ?? "") == nsname)
                        {
-                            names.Add(t.Name);
+                            if (!t.IsNested)


@why is this nested check needed? should there be a test for this?

@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.

den-run-ai · 2018-02-08T03:47:17Z

src/runtime/assemblymanager.cs

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


@dmitriyse what do you mean by transitive dependencies?

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.

@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

den-run-ai · 2018-02-10T03:33:32Z

CHANGELOG.md

@@ -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.


@dmitriyse please add reference to this pull request and issues from which you referred to this pull request.

den-run-ai · 2018-02-10T03:41:21Z

src/runtime/assemblymanager.cs

@@ -196,7 +196,17 @@ public static Assembly LoadAssembly(string name)
            Assembly assembly = null;
            try
            {
-                assembly = Assembly.Load(name);
+                var importEvent = new ImplicitAssemblyLoadingEventArgs(name);


@dmitriyse why did you augment assembly loading with this event, I cannot see the purpose of this.

den-run-ai · 2018-02-10T03:44:32Z

@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.

den-run-ai · 2018-02-10T03:45:56Z

@Cronan thanks for reviewing this!

@dmitriyse if you can come up with a test for nested types, that would be great.

filmor · 2018-08-21T13:13:12Z

To be honest, I don't see the point of the whole event/event-args part of this pull request. SkipAssemblyLoad is never set explicitly, so I fail to see where we actually skip anything. I'll prepare a PR that only includes the IsNested check and GetExportedTypes, that should do the trick.

filmor · 2018-10-29T12:23:29Z

Superceded by #723.

Implicit assembly loading optimization.

8ee0c65

dmitriyse force-pushed the implicit_assembly_load_opt branch from a13e4f6 to e2efdda Compare August 28, 2017 13:14

dmitriyse force-pushed the implicit_assembly_load_opt branch from e2efdda to 9304d72 Compare September 4, 2017 15:23

Assembly loading experience improved + fixes.

d09fc92

dmitriyse force-pushed the implicit_assembly_load_opt branch 2 times, most recently from 09acf53 to e7edeab Compare September 7, 2017 15:51

dmitriyse force-pushed the implicit_assembly_load_opt branch from e7edeab to 1fd5998 Compare September 22, 2017 01:30

dmitriyse requested review from tonyroberts, den-run-ai, filmor and vmuriart October 7, 2017 07:44

dmitriyse mentioned this pull request Nov 2, 2017

Numpy array to C# array #564

Closed

dmitriyse force-pushed the implicit_assembly_load_opt branch from 1fd5998 to e4764f2 Compare November 16, 2017 22:12

dmitriyse force-pushed the implicit_assembly_load_opt branch from e4764f2 to d09fc92 Compare January 12, 2018 18:41

dmitriyse mentioned this pull request Jan 15, 2018

PythonEngine.Initialize slow #598

Closed

dmitriyse mentioned this pull request Jan 28, 2018

Infinite loop/crash in root.LoadNames() #603

Closed

dmitriyse mentioned this pull request Feb 7, 2018

LookupType returns the first type found #622

Closed

4 tasks

den-run-ai reviewed Feb 8, 2018

View reviewed changes

jborbely mentioned this pull request Feb 8, 2018

Not clear exception MSLNZ/msl-loadlib#6

Closed

den-run-ai reviewed Feb 10, 2018

View reviewed changes

Merge branch 'master' into implicit_assembly_load_opt

4af13a3

den-run-ai reviewed Feb 10, 2018

View reviewed changes

den-run-ai and others added 2 commits February 9, 2018 23:53

Merge branch 'master' into implicit_assembly_load_opt

c28bafd

Merge branch 'master' into implicit_assembly_load_opt

f016059

den-run-ai mentioned this pull request Aug 24, 2018

Use GetExportedTypes where possible and filter nested types #723

Merged

4 tasks

filmor closed this Oct 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dmitriyse commented Aug 23, 2017

mention-bot commented Aug 23, 2017

codecov bot commented Aug 23, 2017 •

edited

Loading

den-run-ai Feb 8, 2018

den-run-ai Feb 8, 2018

Cronan Feb 8, 2018

den-run-ai Feb 10, 2018

den-run-ai Feb 10, 2018

den-run-ai Feb 8, 2018

den-run-ai Feb 10, 2018

den-run-ai Feb 8, 2018

Cronan Feb 8, 2018 •

edited

Loading

den-run-ai Feb 10, 2018

den-run-ai Feb 10, 2018

den-run-ai Feb 10, 2018

den-run-ai commented Feb 10, 2018

den-run-ai commented Feb 10, 2018

filmor commented Aug 21, 2018

filmor commented Oct 29, 2018

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

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

Conversation

dmitriyse commented Aug 23, 2017

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

mention-bot commented Aug 23, 2017

codecov bot commented Aug 23, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cronan Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

den-run-ai commented Feb 10, 2018

den-run-ai commented Feb 10, 2018

filmor commented Aug 21, 2018

filmor commented Oct 29, 2018

codecov bot commented Aug 23, 2017 •

edited

Loading

Cronan Feb 8, 2018 •

edited

Loading