Reduce number of instructions when fetching constants#5709
Reduce number of instructions when fetching constants#5709kddnewton wants to merge 1 commit intoruby:masterfrom
Conversation
tenderlove
left a comment
There was a problem hiding this comment.
LGTM, but if we change the instruction shouldn't we also fix YJIT?
|
I might be reading this wrong, but I don't think we generate code for |
|
It's deliberately a stack argument as opposed to an instruction argument for compatibility with tools that look at the byte code. See 661927a and the discussion on https://bugs.ruby-lang.org/issues/11718#note-15. Maybe things have changed since 2019 though. CC @ko1 |
|
Interesting. I see two mentions of “tooling” but I’m not sure what that
means. Is it internal stuff or external stuff? I’ve seen other changes to
the byte code before, do other changes update something else that I missed?
…On Thu, Mar 24, 2022 at 6:09 PM Alan Wu ***@***.***> wrote:
It's deliberately a stack argument as opposed to an instruction argument
for compatibility with tools that look at the byte code. See 661927a
<661927a>
and the discussion on https://bugs.ruby-lang.org/issues/11718#note-15.
Maybe things have changed since 2019 though. CC @ko1
<https://github.com/ko1>
—
Reply to this email directly, view it on GitHub
<#5709 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABG3P3TRBID5NATI362RTUTVBTRZFANCNFSM5RSF3IKA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
https://github.com/ruby/typeprof/ is probably the most prominent tool that relies on the iseq being stable. In fact since it's a bundle gem it's giving CI failures on this PR: https://github.com/ruby/ruby/runs/5682482784?check_suite_focus=true |
|
Ahh I see. Is there something we could version and then pin? This seems
like it’s worth the performance.
…On Thu, Mar 24, 2022 at 7:19 PM Alan Wu ***@***.***> wrote:
https://github.com/ruby/typeprof/ is probably the most prominent tool
that relies on the iseq being stable. In fact since it's a bundle gem it's
giving CI failures on this PR:
https://github.com/ruby/ruby/runs/5682482784?check_suite_focus=true
—
Reply to this email directly, view it on GitHub
<#5709 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABG3P3TWAAYGU4JXXWSLMPTVBT2BXANCNFSM5RSF3IKA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I'm not familiar enough with typeprof to say. Maybe @mame knows. |
|
IMHO not changing the bytecode because some tool(s) relies on it is the beginning of the end (in terms of optimization, flexibility and leaking implementation details). |
|
This change seems like a good idea 👍 I think it's also fairly minor, we'll be able to adapt YJIT easily if we need to change anything. Other tools also shouldn't have a difficult time. |
Currently every time there is a getconstant instruction we push a boolean onto the stack to let it know if it can search the global scope. This boolean is always known at compile-time though, so there's no need for it to be pushed onto the stack. Instead we can pass it directly as an argument to the getconstant instruction, thereby reducing the number of instructions in total.
a623426 to
cf82168
Compare
|
Okay, I've just rebased this branch. In terms of chicken-and-egg, is the only way to get this merged to fix typeprof first so that it can detect this kind of change? Trying to determine how to get this to move forward. |
|
Yeah, we want to fix typeprof first since we want green CI on |
Currently every time there is a getconstant instruction we push a boolean onto the stack to let it know if it can search the global scope. This boolean is always known at compile-time though, so there's no need for it to be pushed onto the stack. Instead we can pass it directly as an argument to the getconstant instruction, thereby reducing the number of instructions in total.
After this change, it appears to be quite a bit faster on the constant invalidation benchmark.