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

Report polymorphism for sprintf nodes. #3537

Merged
merged 3 commits into from
Apr 17, 2024

Conversation

nirvdrum
Copy link
Collaborator

While looking at the performance of a large Rails application, I saw that we were creating new CallTarget on each call to String#%. The generic specialization compiles the sprintf expression from scratch on each call, resulting in the creation of a new call target.

The sprintf code can be invoked in a few different ways, but the one that stood out to me was String#%. Rails will create a request ID for each request to make tracking logs and such easier. The ActiveSupport code for creating the UUID uses a static format string, but the sprintf nodes are already megamorphic by the time this code is called. I saw it go megamorphic by loading the URI library.

I suspect format strings are mostly static and splitting would make most call sites monomorphic. This simple change demonstrably splits with the following example:

def foo(format)
 format % [123]
end

loop do
  foo "%#{rand(3)}d"
  foo "%s"
end

For call sites with > 3 format strings we could add a global cache, like we do for regular expressions. That could cut down on the creation of unique call targets, but that's out of scope for this PR and I don't have any evidence of it being a real world problem at the moment.

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Apr 16, 2024
@@ -1665,6 +1665,7 @@ private static RubyString finishFormat(Node node, int formatLength, BytesResult

Copy link
Member

@eregon eregon Apr 16, 2024

Choose a reason for hiding this comment

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

We should remove the @ReportPolymorphism on SprintfNode above, it has no effect since there is a single specialization instance possible (I filed GR-53454 for Truffle to warn this).
Alternatively we could mark the method as Split.ALWAYS, I think that's a little better because then it just splits eagerly vs only when creating a second specialization instance of formatCached (and force splitting cannot be put on the wrong node like before this PR, unlike @ReportPolymorphism).

Copy link
Member

@eregon eregon Apr 16, 2024

Choose a reason for hiding this comment

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

@andrykonchin Could you integrate this and add an extra commit removing the two @ReportPolymorphism and adding always split on the @CoreMethod?

It seems good to also check other cases of pack/unpack/format/sprintf/etc to make sure they are correct too.
One easy way to find them seems callers of global search RootCallTarget compile(.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, It seems good to keep the @ReportPolymorphism added in this PR, because then if someone does:

def myformat(format, *args)
  sprintf format, *args
end
myformat "%d", 1
myformat "%f", 3.14

it would correctly split, and wouldn't with only the always_split.

But let's always split SprintfNode since we know we'll want to split that one anyway, and the earlier the better for startup/warmup/footprint.

@andrykonchin andrykonchin added the in-ci The PR is being tested in CI. Do not push new commits. label Apr 16, 2024
@graalvmbot graalvmbot merged commit 11f1b26 into oracle:master Apr 17, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-ci The PR is being tested in CI. Do not push new commits. OCA Verified All contributors have signed the Oracle Contributor Agreement. performance shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants