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

[dylink] Remove redundant dlsync call. NFC #20286

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Sep 18, 2023

dlsync is performed before returning from dlopen/dlsym. There is no needed pre-emptively call dlsync at the start of these operations.

dlsync is performed before returning from dlopen/dlsym.  There is no
needed pre-emptively call dlsync at the start of these operations.
@sbc100 sbc100 requested a review from kripken September 18, 2023 23:10
@kripken
Copy link
Member

kripken commented Sep 18, 2023

dlsync is performed before returning from dlopen/dlsym.

Where is that call? Just scanning the rest of the functions here _dlopen and emscripten_dlopen I don't immediately see such a call.

There is no needed pre-emptively call dlsync at the start of these operations.

Can you explain why? Naively, I see a call to find_existing, and I'd guess it's good to sync and see if the library already exists?

@sbc100
Copy link
Collaborator Author

sbc100 commented Sep 19, 2023

dlsync is performed before returning from dlopen/dlsym.

Where is that call? Just scanning the rest of the functions here _dlopen and emscripten_dlopen I don't immediately see such a call.

There is a call to dlsync in __dlsym itself on line 669.

There is a call to dlsync in load_library_done which is call once a dlopen call succeeds.

There is no needed pre-emptively call dlsync at the start of these operations.

Can you explain why? Naively, I see a call to find_existing, and I'd guess it's good to sync and see if the library already exists?

Once we get the write lock with do_write_lock there is now way for us to be out-of-sync with other threads. If another thread calls dlopen it won't release the write lock until all thread are in sync. If we got the write lock the we know the any outstanding sync calls much have completed already.

@kripken
Copy link
Member

kripken commented Sep 19, 2023

I see, thanks!

@sbc100 sbc100 merged commit 94665ee into main Sep 19, 2023
3 checks passed
@sbc100 sbc100 deleted the remove_redundant_dlsync branch September 19, 2023 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants