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

YJIT: Add support for **kwrest parameters #9891

Merged
merged 2 commits into from
Feb 12, 2024
Merged

Conversation

XrXr
Copy link
Member

@XrXr XrXr commented Feb 8, 2024

Now that ... uses **kwrest instead of regular splat and
ruby2keywords, we need to support these type of methods to
support ... well.

On lobsters, this cuts num_send_dynamic by ~1%:

-num_send_dynamic:          1,089,894 ( 4.1%)
+num_send_dynamic:            874,933 ( 3.3%)

Now that `...` uses `**kwrest` instead of regular splat and
ruby2keywords, we need to support these type of methods to
support `...` well.
@XrXr XrXr marked this pull request as ready for review February 9, 2024 00:20
@matzbot matzbot requested a review from a team February 9, 2024 00:20

// Build the keyword rest parameter hash before we make any changes to the order of
// the supplied keyword arguments
if has_kwrest {
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it makes sense to carve out the kwrest logic as a separate function? It's generally hard to follow the sequence of operations in gen_send_iseq, so I wonder if it makes it easier to understand what happens after what in that function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Make sense. I think we should move all the keywords handling out, though. I'll make a PR.

@XrXr XrXr enabled auto-merge (rebase) February 12, 2024 18:54
@XrXr XrXr merged commit 2131d04 into ruby:master Feb 12, 2024
97 checks passed
@XrXr XrXr deleted the yjit-kwrest branch February 12, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants