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

Remove intrusive_list (NFC) #2299

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Remove intrusive_list (NFC) #2299

wants to merge 2 commits into from

Conversation

keithw
Copy link
Member

@keithw keithw commented Sep 17, 2023

This PR replaces the intrusive_list class with a std::list of unique_ptr's, trying to keep everything else mostly the same.

@keithw keithw requested a review from sbc100 September 17, 2023 02:00
@keithw keithw force-pushed the remove-intrusive-list branch 3 times, most recently from ebc4b36 to deb0a58 Compare September 18, 2023 05:54
// * No initializer lists
// * Asserts instead of exceptions
// * Some functions are not implemented (merge, remove, remove_if, reverse,
// unique, sort, non-member comparison operators)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know the rationale for creating this class in the first place? Does the reasoning for it no longer hold?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not super clear (#542), but generally the reason to prefer an intrusive list vs. std::list is about performance (an intrusive_list only needs one allocation to make a list node with both the contained object and the prev/next pointers). And indeed, I just benchmarked this PR (with wasm-validate clang.wasm) and std::list is a significant slowdown for large modules. :-(

main branch:

time ./bin/wasm-validate ~/w2c2/examples/clang/clang.wasm

real	0m1.887s
user	0m1.451s
sys	0m0.436s

this PR:

time ./bin/wasm-validate ~/w2c2/examples/clang/clang.wasm

real	0m2.967s
user	0m2.531s
sys	0m0.436s

I wasn't expecting it to be this bad -- let's hold off on this for now until I find a better way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I originally switched from a vector to an intrusive list, since I thought we'd want to be able to do insertions into the list. I'm not sure that was a good decision, tbh. It may make sense to see whether switching it back to a vector would work.

Copy link
Member Author

@keithw keithw Sep 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, here's a version with std::vector for the ExprList and std::deque for the ModuleFieldList (since we sometimes iterate over the ModuleFieldList while also appending to it...). Still unfortunately a performance regression, but not as bad as before:

$ time ./bin/wasm-validate ~/w2c2/examples/clang/clang.wasm 

real	0m2.456s
user	0m1.972s
sys	0m0.484s

@keithw keithw force-pushed the remove-intrusive-list branch 2 times, most recently from deb0a58 to 0eec053 Compare September 19, 2023 05:42
@keithw keithw marked this pull request as draft September 20, 2023 17:47
@keithw
Copy link
Member Author

keithw commented Sep 20, 2023

Reverting to draft -- I think changing ModuleFieldList away from intrusive_list in a way that doesn't introduce a performance regression will take a more involved change. The Module structure has internal pointers pointing inside the ModuleFieldList, and likes to iterate over the ModuleFieldList while appending to it, so a naive replacement with vector isn't safe (and even deque fails -- in the fuzzer only, not in any of the asan spec tests, which spooks me a bit).

@keithw keithw force-pushed the remove-intrusive-list branch from 0eec053 to 435af28 Compare November 20, 2023 05:24
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.

3 participants