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

Allow --keepfuncs and --splitfuncs to be use alongside a profile data #6322

Merged
merged 13 commits into from
Jul 10, 2024

Conversation

Mintyboi
Copy link
Contributor

There are times after collecting a profile, we wish to manually include specific functions into the primary module.
It could be due to non-deterministic profiling or functions for error scenarios (e.g. _trap).

This PR helps to unlock this workflow by honoring both the --keep-funcs flag as well as the --profile flag

@Mintyboi
Copy link
Contributor Author

@tlively Can you see if this is a suitable improvement that we can include for wasm-split?
If so, do we wish to expand it to the --split-funcs flag as well?

@tlively
Copy link
Member

tlively commented Feb 21, 2024

This makes sense to me! I think we do want to extend it to the --split-funcs flag as well. That way the user can explicitly keep some functions, explicitly split some functions, and depend on the profile to decide what to do for the rest. It would also be good to report a warning if the profile contradicts --split-funcs.

@Mintyboi
Copy link
Contributor Author

Thanks!
I've expanded this usage to the --split-funcs flag as well. The functions in --split-funcs will supersede the other split options and show a warning if it contradicts with the profile data or the keep-funcs

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Just a few comments on the code itself.

Comment on lines 231 to 234

if (splitFuncs.count(func) > 0){
splitFuncs.erase(func);
}
Copy link
Member

Choose a reason for hiding this comment

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

This will already do the right thing if the key is not found:

Suggested change
if (splitFuncs.count(func) > 0){
splitFuncs.erase(func);
}
splitFuncs.erase(func);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in eb3ddfa

src/tools/wasm-split/wasm-split.cpp Outdated Show resolved Hide resolved
src/tools/wasm-split/wasm-split.cpp Show resolved Hide resolved
test/lit/wasm-split/basic.wast Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Mintyboi Mintyboi left a comment

Choose a reason for hiding this comment

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

@tlively I have made some changes, please help to take a look again

src/tools/wasm-split/wasm-split.cpp Show resolved Hide resolved
src/tools/wasm-split/wasm-split.cpp Outdated Show resolved Hide resolved
@Mintyboi Mintyboi changed the title Allow --keepfuncs to be use alongside a profile data Allow --keepfuncs and --splitfuncs to be use alongside a profile data Feb 27, 2024
@Mintyboi
Copy link
Contributor Author

@tlively Thanks for your review and the comments!
Could you help me see if there's anything else I should add in this PR?

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good besides the missing newline and the fix to the warning message

src/tools/wasm-split/wasm-split.cpp Outdated Show resolved Hide resolved
test/lit/wasm-split/basic.wast Outdated Show resolved Hide resolved
Mintyboi and others added 2 commits February 29, 2024 13:08
Co-authored-by: Thomas Lively <tlively123@gmail.com>
@Mintyboi
Copy link
Contributor Author

@tlively Fixed comment and the newline at the eof.

Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Thanks!

@tlively tlively enabled auto-merge (squash) March 12, 2024 17:09
@tlively
Copy link
Member

tlively commented Mar 12, 2024

Looks like there are some formatting issues to fix. You can ignore the failing Emscripten tests, though.

@Mintyboi
Copy link
Contributor Author

@tlively sorry for the delayed response. I'm still interested to get this in if possible.
I'll need some help to trigger the CI so that I can look at the errors

@kripken
Copy link
Member

kripken commented Jun 20, 2024

I restarted the tests now.

auto-merge was automatically disabled July 8, 2024 15:39

Head branch was pushed to by a user without write access

@Mintyboi
Copy link
Contributor Author

Mintyboi commented Jul 8, 2024

I've attempted to fix the linting and the test failures.
Please help to trigger the CI again 🙏

@Mintyboi
Copy link
Contributor Author

@tlively @kripken 1 last trigger I hope, before we can land this PR

@kripken
Copy link
Member

kripken commented Jul 10, 2024

(tests restarted)

@tlively tlively merged commit 76f6612 into WebAssembly:main Jul 10, 2024
13 checks passed
@Mintyboi Mintyboi deleted the wip/ben/enhance-wasmsplit branch July 11, 2024 13:17
@gkdn gkdn mentioned this pull request Aug 31, 2024
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