Skip to content

Inlining: Always inline single-use functions #3730

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

Merged
merged 10 commits into from
Mar 29, 2021
Merged

Inlining: Always inline single-use functions #3730

merged 10 commits into from
Mar 29, 2021

Conversation

kripken
Copy link
Member

@kripken kripken commented Mar 24, 2021

This implements emscripten-core/emscripten#13744

Inlining functions with a single use allows us to remove the function afterward.
That looks highly beneficial, shrinking every single benchmark in emscripten's
benchmark suite, by an average of 2% on the macrobenchmarks and 3.5% on
all of them. Speed also improves, although mostly on the microbenchmarks so
that might be less realistic.

There may be a slight downside to startup time due to emitting larger functions,
but given the baseline compilers in VMs these days it seems worth it, as the
delay would be just to get to the upper tier. On the benchmark suite the risk
seems low.

See more details in the PR above.

@kripken kripken requested review from tlively and aheejin March 24, 2021 23:32
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.

🎉 Well that was simple :)

@MaxGraey
Copy link
Contributor

By the way, it's interesting how much this affects to engine's performance, because in this case there is more register's pressure which may lead to suboptimal register allocation in theory. Maybe better do this only for -O3z?

@kripken
Copy link
Member Author

kripken commented Mar 26, 2021

@MaxGraey That is a risk, yes, but I investigated performance in emscripten-core/emscripten#13744 and did not see such downsides.

One possible reason is that VMs are good enough to register allocate well these days. Another is that inlining is an easy case in that all the inlined code ends up in a single big bundle (where the call was). Unless optimizations move it around, it stays there, and it's easy to register-allocate that well - in the worst case, spilling on entering that bundle and leaving it (which is still not worse than the call). No guarantees of course, but it's less risky than other forms of increasing a function's size.

@kripken kripken merged commit 1a6efdb into main Mar 29, 2021
@kripken kripken deleted the inlineall branch March 29, 2021 18:33
kripken added a commit to emscripten-core/emscripten that referenced this pull request Mar 29, 2021
To allow the binaryen inlining improvement to roll in.

See WebAssembly/binaryen#3730 and #13744
kripken added a commit to emscripten-core/emscripten that referenced this pull request Mar 29, 2021
… traces (#13792)

To allow the binaryen inlining improvement to roll in.

See WebAssembly/binaryen#3730 and #13744
@MaxGraey
Copy link
Contributor

MaxGraey commented Jun 24, 2021

It seems this may affect to performance after recent v8 change:
v8/v8@4722852

@kripken
Copy link
Member Author

kripken commented Jun 29, 2021

Good point @MaxGraey , I'll look into some kind of limit here. We also saw Firefox add a limit recently, so this appears to be a general issue.

kripken added a commit that referenced this pull request Jul 1, 2021
We have seen some cases in both Chrome and Firefox where
extremely large modules cause overhead,

#3730 (comment) (and link therein)
emscripten-core/emscripten#13899 (comment)

There is no "right" value to use as a limit here, but pick an
arbitrary one that is very high. (This setting is verified to have
no effect on the emscripten benchmark suite.)
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.

4 participants