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. #3536

Closed
wants to merge 1 commit into from

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
@nirvdrum nirvdrum added performance shopify and removed OCA Verified All contributors have signed the Oracle Contributor Agreement. labels Apr 16, 2024
@nirvdrum nirvdrum closed this Apr 16, 2024
@nirvdrum nirvdrum deleted the splint-sprintf branch April 16, 2024 06:36
@nirvdrum nirvdrum restored the splint-sprintf branch April 16, 2024 06:37
@nirvdrum nirvdrum deleted the splint-sprintf branch April 16, 2024 06:37
@nirvdrum
Copy link
Collaborator Author

I'm sorry. I noticed a typo in the branch name and used GitHub's UI for renaming the branch. I thought GitHub would treat that more intelligently than manually deleting the branch. But, it did not and when GitHub deleted the branch, it closed this PR. I've reopened as #3537.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant