Skip to content

Conversation

@hahnjo
Copy link
Member

@hahnjo hahnjo commented Nov 18, 2024

Use AbsoluteSymbolsMaterializationUnit to define the symbols.

@hahnjo hahnjo requested a review from vgvassilev November 18, 2024 10:59
@hahnjo hahnjo self-assigned this Nov 18, 2024
@github-actions
Copy link

github-actions bot commented Nov 18, 2024

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.

Use AbsoluteSymbolsMaterializationUnit to define the symbols.
@hahnjo hahnjo force-pushed the cling-autoload-absolute branch from 2c0bc2e to c0fd8cb Compare November 28, 2024 09:28
@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

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?

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.

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!

@hahnjo hahnjo merged commit e154b99 into root-project:master Nov 30, 2024
19 checks passed
@hahnjo hahnjo deleted the cling-autoload-absolute branch November 30, 2024 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants