-
Notifications
You must be signed in to change notification settings - Fork 5.5k
ZJIT: Add ArrayAset instruction to hir #15747
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
Conversation
c3280a3 to
a7e915e
Compare
a7e915e to
5ef58f9
Compare
This comment has been minimized.
This comment has been minimized.
64f31f5 to
a7a557d
Compare
a7a557d to
19d5b63
Compare
|
Thank you for the PR. A couple of notes:
|
|
Also, it seems like the inlined/not-inlined stats did not change much before/after your change. This indicates to me something is wrong here (not being applied fully) |
|
@tekknolagi
Yes, I collected some stats here. It looks like the vast majority of the inline fast-path cases are in-bounds.
Well...this is because there are some cases where ary = [0, 1, 2, 3]
ary[1, 2] = ["a", "b", "c", "d"]
p ary #=> [0, "a", "b", "c", "d", 3]
ary = [0, 1, 2, 3, 4, 5]
ary[0..2] = ["a", "b"]
p ary # => ["a", "b", 3, 4, 5]Would you prefer that I keep this PR focused on the Fixnum-index fast path, and handle slice/range assignment in a separate PR? Or should I expand this PR to cover those cases as well? |
|
Maybe I am being naive, but I would be shocked if range aset was more common than fixnum aset. This feels to me like there is something going wrong with this specialization, meaning it only applies in vanishingly small cases Also, your definition of in-bounds includes negatives (which require adjustment) -- it is important to know the prevalence of positive vs negative indices here |
|
Oh, these are numbers for a very specific benchmark. Let me actually go look at optcarrot, sorry |
|
Ah, yes, I am being naive 😓 def load_tiles
return unless @any_show
@bg_pixels.rotate!(8)
@bg_pixels[@scroll_xfine, 8] = @bg_pattern_lut[@bg_pattern]
endis very hot and the source of our troubles here |
|
Ok, well, we can also look at the loops-times benchmark (which, despite being silly, does have a lot of array+fixnum aset). This PR should be able to get rid of 80MM method calls and turn them into pointer writes |
I have collected the stats which includes positive vs negative indices below, and it seems it's always positive index. in-bounds/out-bounds checkTerms:
Results:
|
This comment was marked as outdated.
This comment was marked as outdated.
|
Super! Looking forward to seeing new guards (length+frozen). I think you can get inspiration from YJIT and other ZJIT codegen snippets for the codegen implementation---if you have questions, please ask. I am happy to help. |
eb080d2 to
8e83e96
Compare
8e83e96 to
80d3d61
Compare
Thank you !! I have pushed the changes in the commit below, and also left some comments and updated the PR description with the latest benchmark results. If you have a moment, I’d really appreciate it if you could take another look. |
tekknolagi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking really good
92c1436 to
c0d9013
Compare
- update test cases so that they are tested in simpler way - rename GuardArrayNotShared -> GuardNotShared - reuse WriteBarrier HIR after ArrayAsetFixnum call - remove GuardInBounds HIR and reuse GuardGreaterEqual and GuardLessThan instructions - rename ArrayAsetFixnum HIR -> ArrayAset
c0d9013 to
934434d
Compare
tekknolagi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! let me know when you are done with these edits and ready to merge
35bab4d to
f5b7bbd
Compare
f5b7bbd to
35f010f
Compare
|
Thanks! Fixed in this commit (35f010f) |
|
Let me know when it's ready to merge |
|
@tekknolagi |
|
Thank you and congratulations! |
|
Thank you so much, Max! I really appreciate the support. |
Inline `Array#[]=` into `ArrayAset`.

Closes: Shopify#804
Benchmark
loops-times
before patch
after patch
optcarrot
before patch
after patch