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

create ifuncs for compiled blocks once at module load time #4905

Merged
merged 12 commits into from Nov 28, 2021

Conversation

froydnj
Copy link
Contributor

@froydnj froydnj commented Nov 23, 2021

Motivation

When a Ruby function rubyfunc receives a block implemented in C, a struct vm_ifunc is created for that block...on every call to rubyfunc. The compiler, of course, does this all the time, and it's suboptimal when compared to the interpreter: the interpreter does not require an extra allocation for passing an (interpreted) block to functions.

This PR intends to change that state of affairs by allocating the struct vm_ifunc up front at module load time. We can then re-use the structure on each call to rubyfunc, saving an allocation at runtime. I expect this to be beneficial for block-heavy benchmarks, but haven't yet gotten the chance to benchmark this.

Test plan

See included automated tests.

@froydnj froydnj requested a review from a team as a code owner November 23, 2021 15:57
@froydnj froydnj requested review from aprocter-stripe and removed request for a team November 23, 2021 15:57
@froydnj
Copy link
Contributor Author

froydnj commented Nov 23, 2021

We have a policy of testing changes to Sorbet against Stripe's codebase before
merging them. I've kicked off a test run for the current PR. When the build
finishes, I'll share with you whether or how it failed. Thanks!

Stripe employees can see the build results here:

https://go/builds/bui_KeKHb3IPQ6znT1
https://go/builds/bui_KeKH1VpF6RB6mS

Copy link
Contributor

@aprocter-stripe aprocter-stripe left a comment

Choose a reason for hiding this comment

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

I think this all seems reasonable! Just one cosmetic suggestion about something that would have made it easier to understand what was going on as I was reviewing.

If you're able, it would be neat to see some benchmarks before and after this change.

@@ -1165,4 +1161,39 @@ llvm::Value *Payload::getCFPForBlock(CompilerState &cs, llvm::IRBuilderBase &bui
llvm::Value *Payload::buildLocalsOffset(CompilerState &cs) {
return llvm::ConstantInt::get(cs, llvm::APInt(64, 0, true));
}

llvm::Value *Payload::buildBlockIfunc(CompilerState &cs, llvm::IRBuilderBase &builder, const IREmitterContext &irctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

I might suggest renaming to getOrBuildBlockIfunc. Working backwards from the call sites I found myself a bit confused that we were "building" an ifunc at certain places, but now I see that this actually uses getOrInsertGlobal.

@froydnj
Copy link
Contributor Author

froydnj commented Nov 23, 2021

We have a policy of testing changes to Sorbet against Stripe's codebase before
merging them. I've kicked off a test run for the current PR. When the build
finishes, I'll share with you whether or how it failed. Thanks!

Stripe employees can see the build results here:

https://go/builds/bui_KeN1EjP7xr7Cpa
https://go/builds/bui_KeN1DVOQAThRCf

@froydnj
Copy link
Contributor Author

froydnj commented Nov 23, 2021

If you're able, it would be neat to see some benchmarks before and after this change.

It's not as effective as I hoped: it does indeed reduce the allocations to interpreter levels on the benchmark that motivated this, but the speedup is in the 1-5% range. Still probably worth it to avoid increasing memory pressure in compiled code?

@froydnj froydnj enabled auto-merge (squash) November 28, 2021 17:34
@froydnj froydnj merged commit b77d2ec into master Nov 28, 2021
@froydnj froydnj deleted the froydnj-split-out-block-func-creation branch November 28, 2021 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants