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.

Sign up for GitHub

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

Jump to bottom

[core] Fold AutoloadLibraryMU into AutoloadLibraryGenerator #16969

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[core] Fold AutoloadLibraryMU into AutoloadLibraryGenerator #16969

wants to merge 1 commit into from
+21 −76

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Nov 18, 2024 •
edited
Loading

Use AbsoluteSymbolsMaterializationUnit to define the symbols.

@hahnjo hahnjo self-assigned this Nov 18, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 18, 2024 •
edited
Loading

Test Results

    18 files      18 suites   3d 19h 47m 57s ⏱️
 2 687 tests  2 687 ✅ 0 💤 0 ❌
46 529 runs  46 529 ✅ 0 💤 0 ❌

Results for commit c0fd8cb.

♻️ This comment has been updated with latest results.

@hahnjo
[core] Fold AutoloadLibraryMU into AutoloadLibraryGenerator
c0fd8cb 
Use AbsoluteSymbolsMaterializationUnit to define the symbols.
@hahnjo hahnjo marked this pull request as ready for review November 28, 2024 09:28
@hahnjo
Copy link
Member Author

hahnjo commented Nov 28, 2024

As far as I could understand, Lang recommends this simplification, so marking as ready for review

vgvassilev
vgvassilev reviewed Nov 28, 2024
View reviewed changes
Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Do we have any performance numbers? Is the absolute symbol approach similar in terms of computational cost?

core/metacling/src/TClingCallbacks.cxx
llvm::orc::SymbolMap loadedSymbols;
for (const auto &KV : found) {
// Try to load the library which should provide the symbol definition.
// TODO: Should this interface with the DynamicLibraryManager directly?
Copy link
Member

Choose a reason for hiding this comment

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

Yes, ideally that'd be a good idea...

@hahnjo
Copy link
Member Author

hahnjo commented Nov 28, 2024

Do we have any performance numbers? Is the absolute symbol approach similar in terms of computational cost?

I'm not sure I understand, this PR is just moving the code from the MU to the Generator. AbsoluteSymbolsMaterializationUnit is literally just doing a notifyResolved of the symbols with their absolute pointers.

@vgvassilev
Copy link
Member

Do we have any performance numbers? Is the absolute symbol approach similar in terms of computational cost?

I'm not sure I understand, this PR is just moving the code from the MU to the Generator. AbsoluteSymbolsMaterializationUnit is literally just doing a notifyResolved of the symbols with their absolute pointers.

I think it answers my question.

vgvassilev
vgvassilev approved these changes Nov 28, 2024
View reviewed changes
Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Lgtm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vgvassilev vgvassilev vgvassilev approved these changes

@dpiparo dpiparo Awaiting requested review from dpiparo dpiparo is a code owner

@pcanal pcanal Awaiting requested review from pcanal pcanal is a code owner

Assignees

@hahnjo hahnjo

Labels
in:Cling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hahnjo @vgvassilev