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.

Sign up for GitHub

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

Jump to bottom

YJIT: Skip Insn::Comment and format! if disasm is disabled #8441

Merged
merged 2 commits into from Sep 14, 2023
Merged

YJIT: Skip Insn::Comment and format! if disasm is disabled #8441

merged 2 commits into from Sep 14, 2023

Conversation

k0kubun
Copy link
Member

@k0kubun k0kubun commented Sep 14, 2023 •
edited

format! is taking some time even on release build. We should skip the call when disasm is not supported.

before: ruby 3.3.0dev (2023-09-14T18:16:21Z master 533c4072a9) +YJIT [x86_64-linux]
after: ruby 3.3.0dev (2023-09-14T20:14:08Z yjit-comment b9cbeee272) +YJIT [x86_64-linux]

----------  -----------  ----------  ---------  ----------  ----------  ---------  -------------  ------------
bench       before (ms)  stddev (%)  RSS (MiB)  after (ms)  stddev (%)  RSS (MiB)  after 1st itr  before/after
30k_ifelse  1225.0       0.0         98.8       1118.6      0.0         98.6       1.10           1.10
----------  -----------  ----------  ---------  ----------  ----------  ---------  -------------  ------------

@k0kubun @XrXr
YJIT: Skip Insn::Comment and format!
4554067 
if disasm is disabled

Co-authored-by: Alan Wu <alansi.xingwu@shopify.com>
@matzbot matzbot requested a review from a team September 14, 2023 19:06
maximecb
maximecb reviewed Sep 14, 2023
View reviewed changes
yjit/src/backend/ir.rs Outdated Show resolved Hide resolved
@maximecb
Copy link
Contributor

Very nice perf gain from such a small change. Nicely done once again!

kddnewton
kddnewton reviewed Sep 14, 2023
View reviewed changes
yjit/src/backend/ir.rs Outdated Show resolved Hide resolved
@k0kubun k0kubun merged commit 982d650 into ruby:master Sep 14, 2023
92 of 94 checks passed
@k0kubun k0kubun deleted the yjit-comment branch September 14, 2023 22:49
@k0kubun
Copy link
Member Author

k0kubun commented Sep 14, 2023

Now Ruby 3.3 YJIT compiles faster than Ruby 3.2 YJIT.

3.2.2: ruby 3.2.2 (2023-03-30 revision e51014f9c0) +YJIT [x86_64-linux]
3.3.0: ruby 3.3.0dev (2023-09-14T22:49:40Z master 982d6503b9) +YJIT [x86_64-linux]

----------  ----------  ----------  ----------  ----------  -------------  -----------
bench       3.2.2 (ms)  stddev (%)  3.3.0 (ms)  stddev (%)  3.3.0 1st itr  3.2.2/3.3.0
30k_ifelse  1165.4      0.0         1127.4      0.0         1.03           1.03
----------  ----------  ----------  ----------  ----------  -------------  -----------

@maximecb
Copy link
Contributor

https://github.com/Shopify/shopify/pull/451618

Wonderful :)

k0kubun added a commit to Shopify/ruby that referenced this pull request Sep 21, 2023
@k0kubun @XrXr
YJIT: Skip Insn::Comment and format! if disasm is disabled (ruby#8441)
394dc78 
* YJIT: Skip Insn::Comment and format!

if disasm is disabled

Co-authored-by: Alan Wu <alansi.xingwu@shopify.com>

* YJIT: Get rid of asm.comment

---------

Co-authored-by: Alan Wu <alansi.xingwu@shopify.com>
@k0kubun k0kubun mentioned this pull request Sep 21, 2023
Merged
k0kubun added a commit to Shopify/ruby that referenced this pull request Sep 25, 2023
@k0kubun @XrXr
YJIT: Skip Insn::Comment and format! if disasm is disabled (ruby#8441)
7559cff 
* YJIT: Skip Insn::Comment and format!

if disasm is disabled

Co-authored-by: Alan Wu <alansi.xingwu@shopify.com>

* YJIT: Get rid of asm.comment

---------

Co-authored-by: Alan Wu <alansi.xingwu@shopify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kddnewton kddnewton kddnewton left review comments

@XrXr XrXr XrXr left review comments

@maximecb maximecb maximecb approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
4 participants
@k0kubun @maximecb @kddnewton @XrXr