Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove IAssemblyName (and various fusion remnants) #50755

Merged
merged 8 commits into from
Apr 7, 2021

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Apr 5, 2021

Remove a bunch of remnants of fusion from the binder. Definitely lots more that this change doesn't deal with (e.g. the existence of the ICLRPrivBinder interface - it is internal only and our usage of if it already doesn't adhere to COM semantics).

  • Replace IAssemblyName with a simple struct (AssemblyNameData) for passing information to binder instances. (If we get rid of the interface per above, we can remove the intermediary struct as well - but it is at least simpler and less costly than IAssemblyName.)
  • Delete a bunch of files / functions that were just for dealing with IAssemblyName
  • Move some types from deleted files to bindertypes.hpp: PEKIND from peinformation.h, AssemblyContentType and ASM_DISPLAY_FLAGS from fusion.h

This avoids re-parsing of the display name and some data copying, so it actually improves perf some too. Relative to the cost of actually loading an assembly, it is small / lost in the noise, so I tried narrowing down a measurement to the cost of just going through bind resolution by using custom ALCs and calling LoadFromAssemblyName for an already loaded assembly.

Stopwatch timings for 150k iterations using my local build:

Before - 150k iterations (s) After - 150k iterations (s)
Returned by ALC.Load 1.8 0.9
Fall back to default ALC 1.9 1.0
Test code
Assembly asm = Assembly.GetExecutingAssembly();
AssemblyName asmName = asm.GetName();
int iterations = 150000;
AssemblyLoadContext[] alcs = new AssemblyLoadContext[iterations];
for (int i = 0; i < alcs.Length; i++)
{
    alcs[i] = ...
}

Stopwatch stopwatch = new Stopwatch();
stopwatch.Start();
for (int i = 0; i < alcs.Length; i++)
{
    alcs[i].LoadFromAssemblyName(asmName);
}
stopwatch.Stop();

For returning assembly in Load:

alcs[i] = new CustomALC()
...
class CustomALC : AssemblyLoadContext
{
    protected override Assembly Load(AssemblyName assemblyName)
    {
        return assemblyName.Name == asmName.Name ? asm : null;
    }
}

For fallling back to default ALC:

alcs[i] = new AssemblyLoadContext("DefaultALCFallback")

@ghost
Copy link

ghost commented Apr 5, 2021

Tagging subscribers to this area: @vitek-karas, @agocke, @CoffeeFlux, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details
Author: elinor-fung
Assignees: -
Labels:

area-AssemblyLoader-coreclr

Milestone: -

@elinor-fung elinor-fung changed the title Remove IAssemblyName Remove IAssemblyName (and various fusion remnants) Apr 6, 2021
@elinor-fung elinor-fung marked this pull request as ready for review April 6, 2021 05:51
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

I assume we have to use normal pointer type to pass around the new data structure because it goes through one COM-like interface...
Regardless I think it would be nice if we could make it a reference in all other places - it is stack allocated (only - which is great). Maybe even a const reference if possible.

src/coreclr/binder/clrprivbinderassemblyloadcontext.cpp Outdated Show resolved Hide resolved
@vitek-karas
Copy link
Member

I love this change - delete bunch of code, remove complexity... and get a perf win ...

@AaronRobinsonMSFT
Copy link
Member

@stephentoub based on his love of perf wins

src/coreclr/binder/assembly.cpp Outdated Show resolved Hide resolved
src/coreclr/binder/assemblyname.cpp Show resolved Hide resolved
@elinor-fung
Copy link
Member Author

Merging, since the latest commit was non-functional changes and all the CI legs passed before it, so I don't think I need to add to the current PR/CI backlog.

@elinor-fung elinor-fung merged commit f6b344b into dotnet:main Apr 7, 2021
@elinor-fung elinor-fung deleted the lessFusion branch April 7, 2021 04:11
thaystg added a commit to thaystg/runtime that referenced this pull request Apr 7, 2021
* upstream/main: (568 commits)
  [wasm] Set __DistroRid on Windows to browser-wasm (dotnet#50842)
  [wasm] Fix order of include paths, to have the obj dir first (dotnet#50303)
  [wasm] Fix debug build of AOT cross compiler (dotnet#50418)
  Fix outdated comment (dotnet#50834)
  [wasm][tests] Add properties to allow passing args to xharness (dotnet#50678)
  Vectorized common String.Split() paths (dotnet#38001)
  Fix binplacing symbol files. (dotnet#50819)
  Move type check to after the null ref branch in out marshalling of blittable classes. (dotnet#50735)
  Remove extraneous CMake version requirement. (dotnet#50805)
  [wasm] Remove unncessary condition for EMSDK (dotnet#50810)
  Add loop alignment stats to JitLogCsv (dotnet#50624)
  Resolve ILLink warnings in System.Diagnostics.DiagnosticSource (dotnet#50265)
  Avoid unnecessary closures/delegates in Process (dotnet#50496)
  Fix for field layout verification across version bubble boundary (dotnet#50364)
  JIT: Enable CSE for VectorX.Create (dotnet#50644)
  [main] Update dependencies from mono/linker (dotnet#50779)
  [mono] More domain cleanup (dotnet#50771)
  Race condition in Mock reference tracker runtime with GC. (dotnet#50804)
  Remove IAssemblyName (and various fusion remnants) (dotnet#50755)
  Disable failing test for GCStress. (dotnet#50828)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators May 7, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants