partial fix 44948 #47269
partial fix 44948 #47269
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
src/coreclr/vm/prestub.cpp
Outdated
| { | ||
| AppDomain::AssemblyIterator it = SystemDomain::System()->DefaultDomain()->IterateAssembliesEx((AssemblyIterationFlags)(kIncludeLoaded|kIncludeExecution)); | ||
| CollectibleAssemblyHolder<DomainAssembly *> pDomainAssembly; | ||
| while (it.Next(pDomainAssembly.This())) |
There was a problem hiding this comment.
We do see important applications that load 1000+ assemblies. I suspect that iterating through 1000+ assemblies before JITing each method will show up as unacceptable performance regression.
There was a problem hiding this comment.
@y-yamshchikov A hashmap might reduce looping time. Please refer to FnV code.
There was a problem hiding this comment.
@HJLeee for creating hasmap we have to make global table and add new entries on every assembly loading. It will take time (may be not even big) and increase memory consumption (otherwise we'll have to change R2R format). Will recheck FNV if current approach will not give enough gain.
In case of Large Version Bubble we should to search compiled instantiation of external generic method in assembly where it's being called first. It should help with many case related to #46160. So now we're implementing this logic in ExternalMethodFixupWorker.
|
@davidwrighton Do you have thoughts about the best way to address this? |
y-yamshchikov
force-pushed
the
partial-fix-44948
branch
from
548870d
to
7b521d4
Compare
|
There is an improvement of the previous approach. We eliminated iteration through modules in an appdomain. We guess that if we have a call from a method defined in some module A to a generic method defined in module B then in version bubble scenario highly likely the R2R code resides in module A, and Here we add a way to |
|
A lot of tests are failing. I think the problem is the recursive call of |
|
Hi @y-yamshchikov, since this is not quite ready to merge could you please convert to draft or close till a full fix is available? |
y-yamshchikov
force-pushed
the
partial-fix-44948
branch
from
7b521d4
to
660c697
Compare
|
I've recently pushed an improved version of the feature. This PR uses the ability of the runtime to determine caller site via Dear colleagues @jkotas, @mangod9, @alpencolt, please take a look. |
| Module* pModule = NULL; | ||
| FrameWithCookie<ExternalMethodFrame> frame(pTransitionBlock); | ||
| ExternalMethodFrame * pEMFrame = &frame; | ||
| pModule = ExecutionManager::FindReadyToRunModule((TADDR)(((BYTE*)pEMFrame->GetReturnAddress())-1)); |
There was a problem hiding this comment.
This is again a linear search through all loaded assemblies before JITing every method.
There was a problem hiding this comment.
Would it be a suitable approach to improve FindReadyToRunModule to do O(log(N)) or even O(1) search on some structured cache?
ghost
closed this
|
Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it. |
ghost
locked as resolved and limited conversation to collaborators
This code simply traverses through assemblies in the application
domain. For each assembly (module) it realizes is it Ready To Run and is
it in the same bubble with (does it deliberately bubbling the) module
from which generic function originates. If so, it makes request for code
is Ready To Run and hopes there is some in the module. If the request
succeeds it proceeds with found pointer to the bare native code.
This PR fixes a part of 44948 issue:
Now such methods use their Ready To Run code.
This PR fixes #46160
@jkotas @alpencolt @gbalykov @t-mustafin
Also we have got significant performance gain on startup, that measures between 4.5% to 10% depending on an app.