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

Dynlink: run _shared_startup only once per plugin #2297

Merged
merged 2 commits into from Mar 7, 2019

Conversation

Projects
None yet
4 participants
@nojb
Copy link
Contributor

commented Mar 7, 2019

This fixes a bug with #1063, which mistakenly runs the _shared_startup code for each unit in the .cmxs being loaded. This caused huge slowdowns when dynlinking a plugin with many units, and maybe worse.

This should go in 4.08.

@mshinwell @lpw25

nojb added some commits Mar 7, 2019

@nojb nojb added the bug label Mar 7, 2019

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

OK once CI passes. Apologies.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

@damiendoligez Please confirm that we can cherry-pick this to 4.08.

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Thanks for the quick review!

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

CI green, merging and waiting for confirmation from @damiendoligez before cherry-picking.

@nojb nojb merged commit 9dda8fa into ocaml:trunk Mar 7, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nojb nojb deleted the nojb:fix_dynlink_shared_startup branch Mar 7, 2019

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

Note: fix for nodynlink.ml pushed directly to trunk in 3bde063.

(also needs to be cherry-picked).

@shindere

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2019

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

@shindere Thanks, but the PR has not yet been actually cherry-picked, it is waiting for confirmation from @damiendoligez. Sorry for the confusion!

@shindere

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2019

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

OK for 4.08

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Done in 705b5ba, c5a6755 and 6f7212d. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.