-
Notifications
You must be signed in to change notification settings - Fork 623
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
base: main
Are you sure you want to change the base?
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
core/iwasm/aot/aot_loader.c
Outdated
static int | ||
cmp_export_name(const void *a, const void *b) | ||
{ | ||
return strcmp(((WASMExport *)a)->name, ((WASMExport *)b)->name); |
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.
Should be *(char **)a)
, *(char **)b
?
error_buf_size); | ||
} | ||
|
||
if (!names) { |
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.
had better put check just after names = loader_malloc
core/iwasm/interpreter/wasm_loader.c
Outdated
static int | ||
cmp_export_name(const void *a, const void *b) | ||
{ | ||
return strcmp(((WASMExport *)a)->name, ((WASMExport *)b)->name); |
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.
had better *(char **)a
error_buf_size); | ||
} | ||
|
||
if (!names) { |
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.
same as above
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.
45428c1
to
1c32732
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. Export order is preserved.