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

Inline AddNode specialization that are not yet inlined into InlinedAddNode #2518

Merged
merged 7 commits into from
Oct 29, 2021

Conversation

smarr
Copy link
Contributor

@smarr smarr commented Oct 28, 2021

This change improves interpreter speed on microbenchmarks like Mandelbrot, Sieve, Storage, reducing run time by 7%.
More interesting benchmarks, don't really show any change.

Performance report here: https://rebench.stefan-marr.de/compare/TruffleRuby/069b9e68efef1a09f6fa2a2c722059af43e300ae/60c3282176185aa99c0f1ab8c727c6e6822c8513#ruby-startup-TruffleRuby-native-interp

The drawback is that this duplicates logic from the AddNode. Though, the logic duplicated seems trivial, and the double logic was already duplicated to start with.

…dNode

Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

This looks good, thanks, I'll also do it for InlinedSubNode for consistency.

In general Inlined*Node do inline the logic if it's trivial (hence it was done for double), here it's not fully trivial but still reasonably simple and short (two 1-line specializations). I'll probably cross-link them via JavaDoc to keep them in sync (e.g., the addWithOverflow(long, long) could probably do something better by doing a fast-path overflow check in the future).

@Specialization(assumptions = "assumptions")
protected Object intAdd(int a, int b) {
return getAddNode().executeAdd(a, b);
@Specialization(rewriteOn = ArithmeticException.class)
Copy link
Member

Choose a reason for hiding this comment

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

There is one bug here that each of these specializations should check the Assumptions with assumptions = "assumptions".

@eregon eregon force-pushed the faststart/inlined-add branch from 4d14308 to fd203cf Compare October 29, 2021 12:40
@eregon eregon added the in-ci The PR is being tested in CI. Do not push new commits. label Oct 29, 2021
@eregon eregon added this to the 22.0.0 milestone Oct 29, 2021
@smarr
Copy link
Contributor Author

smarr commented Oct 29, 2021

Hm, so, not sure whether this is a huge win overall.
https://rebench.stefan-marr.de/compare/TruffleRuby/069b9e68efef1a09f6fa2a2c722059af43e300ae/fd203cf9f8a1f3351ee64c863d42d8b9ff441050#ruby-startup-TruffleRuby-native-interp

Results are generally indicating a reduction of interpreter run time, but it's small I'd think.

@eregon
Copy link
Member

eregon commented Oct 29, 2021

Maybe the Assumption[] checks are not so cheap (1 volatile read per Assumption + iterating the array).
I think it might be possible to reduce it to checking a single Assumption (right now it's 3, 2 for Integer#+ and Float#+, and 1 for set_trace_func).
For set_trace_func it might be possible to rely on instrumentation to replace with a full call node as needed with materializeInstrumentableNodes().

@graalvmbot graalvmbot merged commit fd203cf into oracle:master Oct 29, 2021
graalvmbot pushed a commit that referenced this pull request Oct 29, 2021
…o InlinedAddNode (#2518)

PullRequest: truffleruby/3022
@smarr smarr deleted the faststart/inlined-add branch November 17, 2021 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. oca-signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants