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

Eliminate 1-2 array allocations for each splat used in a op_asgn method #9725

Merged
merged 1 commit into from
Jan 27, 2024

Conversation

jeremyevans
Copy link
Contributor

Given code such as:

  h[*a, 1] += 1
  h[*b] += 2

Ruby would previously allocate 5 arrays:

  • splatarray true for a
  • newarray for 1
  • concatarray for [*a, 1] and [1]
  • newarray for 2
  • concatarray for b and [2]

This optimizes it to only allocate 2 arrays:

  • splatarray true for a
  • splatarray true for b

Instead of the newarray/concatarray combination, pushtoarray is used.

Note above that there was no splatarray true for b originally. The normal compilation uses splatarray false for b. Instead of trying to find and modify the splatarray false to splatarray true, this adds splatarray true for b, which requires a couple of swap instructions, before the pushtoarray. This could be further optimized to remove the need for those three instructions, but I'm not sure if the complexity is worth it.

Additionally, this sets VM_CALL_ARGS_SPLAT_MUT on the call to []= in the h[*b] case, so that if []= has a splat parameter, the new array can be used directly, without additional duplication.

Given code such as:

```ruby
  h[*a, 1] += 1
  h[*b] += 2
```

Ruby would previously allocate 5 arrays:

* splatarray true for a
* newarray for 1
* concatarray for [*a, 1] and [1]
* newarray for 2
* concatarray for b and [2]

This optimizes it to only allocate 2 arrays:

* splatarray true for a
* splatarray true for b

Instead of the newarray/concatarray combination, pushtoarray is used.

Note above that there was no splatarray true for b originally. The
normal compilation uses splatarray false for b.  Instead of trying
to find and modify the splatarray false to splatarray true, this
adds splatarray true for b, which requires a couple of swap
instructions, before the pushtoarray.  This could be further
optimized to remove the need for those three instructions, but I'm
not sure if the complexity is worth it.

Additionally, this sets VM_CALL_ARGS_SPLAT_MUT on the call to
[]= in the h[*b] case, so that if []= has a splat parameter, the
new array can be used directly, without additional duplication.
@jeremyevans jeremyevans requested a review from nobu January 26, 2024 23:24
Copy link
Member

@nobu nobu left a comment

Choose a reason for hiding this comment

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

Seems repeating the patterns very similar to #9723.
Any idea to deduplicate?

@jeremyevans
Copy link
Contributor Author

Seems repeating the patterns very similar to #9723. Any idea to deduplicate?

Similar in idea, but fairly different in terms of code:

Flag modification:

  • masgn: generate new ci using ci_flag_set
  • op_asgn: modify flag directly using flag |=

Instruction addition:

  • masgn: Uses INSERT_BEFORE_INSN{,1}
  • op_asgn: Uses ADD_INSN{,1}

I couldn't think of a good way to reuse the logic. It may be best to keep them separate.

For this PR (op_asgn), we could use a couple macros to deduplicate some of the code. Let me know if you would like me to do that.

@nobu
Copy link
Member

nobu commented Jan 27, 2024

For this PR (op_asgn), we could use a couple macros to deduplicate some of the code. Let me know if you would like me to do that.

Yeah, I think it might be possible by macros probably too but would not be needed for now.

@jeremyevans jeremyevans merged commit a591e11 into ruby:master Jan 27, 2024
98 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants