-
Notifications
You must be signed in to change notification settings - Fork 627
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
Fix quadratic runtime for duplicate export name detection #3861
Fix quadratic runtime for duplicate export name detection #3861
Conversation
d9f2ffb
to
1348e5b
Compare
In a module with 100,000 exports, the load time goes from 20+ seconds to a couple hundred milliseconds on my machine. In the case of 10,000 exports, it's a couple hundred milliseconds down to tens of milliseconds. This change will qsort the array once at the end of export section loading. I was initially hoping that sorting the exports themselves by name would allow a binary search implementation in all the |
OK, got it, it really improves a lot. It only allocates memory one time and qsort the array one time.
Keeping |
#3865 was more like my first approach of sorting the exports in place, but I couldn't figure out why the CI was failing. You've fixed that, so I think that approach is obviously better |
But #3865 only optimizes wasm_runtime_lookup_function, it doesn't refine the wasm/aot loading process. I am not sure why you close this PR? The idea looks good. |
I misunderstood your other change, I thought you had a better way to fix performance in the loader as well, I'll reopen this one and address your comments. |
3b24135
to
2e5925c
Compare
@wenyongh This should be good for another look, thank you for your comments |
617acb8
to
45428c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor comments
1c32732
to
980593e
Compare
Previously, the loader would check the name of a new export against all existing exports, leading to a quadratic running time. This change makes the loader parse the entire export section. The exports are then sorted by name, then adjacent exports are checked for uniqueness.
980593e
to
dd728a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…liance#3861) Previously, the loader would check the name of a new export against all existing exports, leading to a quadratic running time. This change makes the loader parse the entire export section. The exports are then sorted by name, then adjacent exports are checked for uniqueness.
Previously, the loader would check the name of a new export against all existing exports, leading to a quadratic running time.
This change makes the loader parse the entire export section. The exports are then sorted by name, then adjacent exports are checked for uniqueness. Export order is preserved.