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

Fix quadratic runtime for duplicate export name detection #3861

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sjamesr
Copy link
Contributor

@sjamesr sjamesr commented Oct 16, 2024

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.

@sjamesr
Copy link
Contributor Author

sjamesr commented Oct 16, 2024

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 wasm_runtime_lookup_... functions, but as you observed in #3407, this causes other problems. Your solution would require keeping 2*ExportCount pointers in memory while the module is loaded, is this OK?

@wenyongh
Copy link
Contributor

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.

OK, got it, it really improves a lot. It only allocates memory one time and qsort the array one time.

I was initially hoping that sorting the exports themselves by name would allow a binary search implementation in all the wasm_runtime_lookup_... functions, but as you observed in #3407, this causes other problems. Your solution would require keeping 2*ExportCount pointers in memory while the module is loaded, is this OK?

Keeping 2 * export_count pointers really consumes a lot of memory. IIUC, maybe we can just use a uint32 array (e.g. uint32 *export_iths_of_sorted_name) to save each export's ith of a temporary sorted export array, which is sorted by name. And then use binary-search to lookup to export, and get the export name with exports[export_iths_of_sorted_name[i] in the lookup.

core/iwasm/interpreter/wasm_loader.c Outdated Show resolved Hide resolved
core/iwasm/interpreter/wasm_loader.c Outdated Show resolved Hide resolved
core/iwasm/interpreter/wasm_loader.c Outdated Show resolved Hide resolved
core/iwasm/interpreter/wasm_loader.c Show resolved Hide resolved
@wenyongh
Copy link
Contributor

@sjamesr I got an idea to improve the performance of wasm_runtime_lookup_function and submitted PR #3865, could you help check it? Thanks.

@sjamesr
Copy link
Contributor Author

sjamesr commented Oct 17, 2024

#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

@sjamesr sjamesr closed this Oct 17, 2024
@wenyongh
Copy link
Contributor

#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.

@sjamesr
Copy link
Contributor Author

sjamesr commented Oct 18, 2024

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.

@sjamesr sjamesr reopened this Oct 18, 2024
@sjamesr sjamesr force-pushed the fix_quadratic_dup_detection branch 4 times, most recently from 3b24135 to 2e5925c Compare October 18, 2024 20:29
@sjamesr
Copy link
Contributor Author

sjamesr commented Oct 19, 2024

@wenyongh This should be good for another look, thank you for your comments

core/iwasm/interpreter/wasm_loader.c Outdated Show resolved Hide resolved
core/iwasm/interpreter/wasm_loader.c Outdated Show resolved Hide resolved
@sjamesr sjamesr force-pushed the fix_quadratic_dup_detection branch 3 times, most recently from 617acb8 to 45428c1 Compare October 19, 2024 16:33
Copy link
Contributor

@wenyongh wenyongh left a 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

static int
cmp_export_name(const void *a, const void *b)
{
return strcmp(((WASMExport *)a)->name, ((WASMExport *)b)->name);
Copy link
Contributor

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) {
Copy link
Contributor

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

static int
cmp_export_name(const void *a, const void *b)
{
return strcmp(((WASMExport *)a)->name, ((WASMExport *)b)->name);
Copy link
Contributor

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) {
Copy link
Contributor

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.
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