Page MenuHomePhabricator

[RuntimeDyld] Speedup resolution of relocations to external symbols
ClosedPublic

Authored by maksfb on Feb 25 2021, 11:59 PM.

Details

Summary

From what I can tell, the loop inside applyExternalSymbolRelocations()
used to call getSymbolAddress(). After the JITSymbolResolver interface
redesign, the functionality has changed, and the loop should no longer
trigger repopulation of ExternalSymbolRelocations. If that's the case,
there is no need to update the loop iterator manually, and
ExternalSymbolRelocations can be cleared at once. This way, when there
are many external symbols in the program, the function runs much faster.

Diff Detail

Event Timeline

maksfb created this revision.Feb 25 2021, 11:59 PM
maksfb requested review of this revision.Feb 25 2021, 11:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2021, 11:59 PM
maksfb added a comment.Mar 3 2021, 9:01 PM

@lhames: does this diff make sense to you or I missed a way ExternalSymbolRelocations can get altered during the loop execution?

After the JITSymbolResolver interface redesign, the functionality has changed, and the loop should no longer trigger repopulation of ExternalSymbolRelocations.

That API change happened a while ago so my recollection may be wrong, but I don't think it changed the fundamental fact that lookup could trigger linking of additional modules. If that's the case I think we could still end up with additional externals needing to be resolved.

Side note: this issue is fixed in ORCv2 / JITLink.

rafauler added inline comments.Mar 10 2021, 4:42 PM
llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
1140

That (triggering linking of additional modules) is accomplished here on this loop inside RuntimeDyldImpl::resolveExternalSymbols()

It doesn't look like applyExternalSymbolRelocations() (the previous function) needs to expect that ExternalSymbolMap will be changed while it is iterating over it (see the call in line 1188 happening only after all modules have been loaded).

lhames accepted this revision.Mar 11 2021, 2:20 PM

Oh, that getSymbolAddress call was moved out of the loop. Ok -- Looks good to me.

Thanks for this Maksim, and for the clarification Rafael.

  • Lang.
This revision is now accepted and ready to land.Mar 11 2021, 2:20 PM