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

[WIP] Loading assembly references in parallel #601

Closed
wants to merge 1 commit into from

Conversation

fadimounir
Copy link
Contributor

@fadimounir fadimounir commented Dec 5, 2019

While debugging multiple issues with crossgen2, i started noticing that the loading of references was slightly slow, especially that we reference *.dll from core_root and the test directory now.

On a cold run of crossgen2 to compile some random test and run it, it took 7292ms for just loading the references.

With the proposed changes, it takes only 900ms, so it's shaving off a good ~88% of the loading time on 8 cores. Multiply that by the number of P0/P1 tests, I believe this small win could be quite beneficial.

Thoughts?

@dotnet/crossgen-contrib

@trylek
Copy link
Member

trylek commented Dec 5, 2019

I'm somewhat worried that @MichalStrehovsky is probably the only person that can assess whether your proposed change is correct according to the CoreRT compiler design.

@fadimounir
Copy link
Contributor Author

fadimounir commented Dec 5, 2019

Unless i'm missing something, the work that seems to happen is metadata loading using the System.Reflection.MEtadata APIs. Places where we save what we loaded into a data structure have locks

@jkotas
Copy link
Member

jkotas commented Dec 6, 2019

The CoreRT compiler design is loading all these lazily on demand. The only reason why we are doing this eager loading here is ProfileDataManager. Can we do this eager loading only when there are profile data for ProfileDataManager to use? Not doing the work would be superior to doing the work in parallel.

@fadimounir
Copy link
Contributor Author

That's a good point, it does look like the eager loading is only tied to ProfileDataManager. I'll address that issue.

If we have profile data, is there a reason to not do this eager loading with parallelism?

@jkotas
Copy link
Member

jkotas commented Dec 6, 2019

If we have profile data, is there a reason to not do this eager loading with parallelism?

That sounds fine.

@fadimounir fadimounir changed the title Loading assembly references in parallel [WIP] Loading assembly references in parallel Dec 6, 2019
@davidwrighton
Copy link
Member

Also that ProfileDataManager is doing a lot of this loading in the assumption that it will be allowed to use the data. Mostly that isn't true, and we really should tweak what stuff is actually loaded.

@fadimounir
Copy link
Contributor Author

cc @SrivastavaAnubhav if you'd like to pick that one up, since you were working on performance :)

@fadimounir fadimounir closed this Feb 4, 2020
@fadimounir fadimounir deleted the ParallelLoadRefs branch March 7, 2020 01:03
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
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.

None yet

5 participants