8000 Skip checking block type in sorbet-runtime when possible by jez · Pull Request #6487 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Skip checking block type in sorbet-runtime when possible #6487

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Oct 24, 2022
Merged

Skip checking block type in sorbet-runtime when possible #6487

merged 6 commits into from
Oct 24, 2022

Conversation

jez
Copy link
Collaborator
@jez jez commented Oct 18, 2022 •
edited
Loading

Motivation

Runtime performance

Benchmarks

I've collected these before fixing the aforementioned problems with nil, so
it's possible that these benchmarks will get slower after that.

Only the bolded things should get sped up from this change—other changes are included to showcase how noisy the measurements usually are. Speedups are reported as percentage speedups vs "Before" (so negative percents are slowdowns—some things slow down by small percentages due to noise in measurement).

Benchmark (all times in ns) Before w/o nil checks % faster w/ nil checks % faster
Vanilla Ruby method call 15 15 0% 15 0%
Vanilla Ruby is_a? 25 24 4% 26 -4%
T::Types::Simple#valid? 37 37 0% 38 -3%
T.nilable(Integer).valid? 49 49 0% 51 -4%
T.any(Integer, Float, T::Boolean).valid? 182 180 1% 180 1%
T.let(..., Integer) 289 288 0% 294 -2%
sig {params(...).void} 198 210 -6% 198 0%
sig {params(..., blk: T.proc.void)} -- {} 997 808 23% 807 24%
sig {params(..., blk: T.proc.void)} -- &blk 849 663 28% 657 29%
sig {params(..., blk: T.nilable(T.proc.void))} -- {} 1044 780 34% 754 38%
sig {params(..., blk: T.nilable(T.proc.void))} -- &blk 937 660 42% 651 44%
T.let(..., T.nilable(Integer)) 733 737 -1% 741 -1%
sig {params(x: T.nilable(Integer)).void} 223 224 0% 221 1%
T.let(..., Example) 294 292 1% 297 -1%
sig {params(x: Example).void} 191 192 -1% 189 1%
T.let(..., T.nilable(Example)) 736 723 2% 739 0%
sig {params(x: T.nilable(Example)).void} 223 225 -1% 223 0%
sig {params(s: Symbol, x: Integer, y: Integer).void} (with kwargs) 1413 1412 0% 1389 2%
direct call Object#class 27 29 -7% 28 -4%
.bind(example).call Object#class 182 188 -3% 182 0%
.bind_call(example) Object#class 102 106 -4% 101 1%

Update

I pushed a change to make the optimization also combine with the existing fast path optimizations (because those validators already skip the block type checking, so when the method mentions a &blk parameter but there is no need to check the block type, we can still take the fast path validators). As expected, the fast path validators are quite fast.

Benchmark (all times in ns) w/ nil and take fast path if possible % faster
sig {params(...).void} 206 -4%
sig {params(..., blk: T.proc.void)} -- {} 819 22%
sig {params(..., blk: T.proc.void)} -- &blk 689 23%
sig {params(..., blk: T.nilable(T.proc.void))} -- {} 309 238%
sig {params(..., blk: T.nilable(T.proc.void))} -- &blk 236 298%

This makes passing a {} block within 50% of no block, and passing a &blk block within 15% of no block.

Analysis

Oddly, the "without nil checks" times are actually faster than the initial implementation that did not check and report a sorbet-runtiem error for nil. I take that to mean that there is likely, there is no additional performance gained by skipping the nil checking when possible, but I figure we may as well keep the separate code paths, because it also means that there's not a penalty to doing so.

Test plan

See included automated tests.

old, outdated initial description

Leaving this as work-in-progress, because the one thing that the VM doesn't do
that Sorbet does do currently is check whether a block was required.

One approach would be to only skip this check for methods whose block is typed
as T.nilable(...), and simply leave the old behavior otherwise.

Another approach would be to do something like the create_validator_fast
methods that skips checking block types for all methods (not just those that
have a certain arity) depending on whether they take a block or not.

@jez jez changed the base branch from master to jez-reject-non-proc October 18, 2022 23:07
This was referenced Oct 18, 2022
Merged
Merged
@jez
Copy link
Collaborator Author
jez commented Oct 18, 2022

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@jez jez force-pushed the jez-reject-non-proc branch from 72cf5c9 to 5cebbe3 Compare October 18, 2022 23:09
@jez jez force-pushed the jez-no-check-block-arg branch 3 times, most recently from cec4a41 to fe31580 Compare October 19, 2022 19:03
@jez jez changed the title wip: Stop checking block type in sorbet-runtime Skip checking block type in sorbet-runtime when possible Oct 19, 2022
@jez jez marked this pull request as ready for review October 19, 2022 19:04
@jez jez requested a review from a team as a code owner October 19, 2022 19:04
@jez jez requested review from froydnj and removed request for a team October 19, 2022 19:04
@jez jez force-pushed the jez-reject-non-proc branch from 8f134e7 to f13ff3e Compare October 20, 2022 19:02
@jez jez force-pushed the jez-no-check-block-arg branch 2 times, most recently from 931c194 to 02b378a Compare October 20, 2022 19:58
Base automatically changed from jez-reject-non-proc to master October 20, 2022 20:16
@jez jez force-pushed the jez-no-check-block-arg branch 2 times, most recently from ae4c780 to 2ee62f0 Compare October 24, 2022 18:24
jez added 5 commits October 24, 2022 13:19
@jez
Fix benchmark
131cb05 
`time_block` assumes the thing under test happens twice, unless provided
an optional argument saying otherwise.
@jez
Add integer_param_and_block benchmark
fb2a45c
@jez
Stop checking block_type in sorbet-runtime
0934cea 
The Ruby VM does this for us.
@jez
With proper nil checking
4a013ef
@jez
Allow fast path validators to fire when we can skip block type
3ebd808
@jez jez force-pushed the jez-no-check-block-arg branch from 2ee62f0 to 3ebd808 Compare October 24, 2022 20:19
@jez jez enabled auto-merge (squash) October 24, 2022 22:04
ilyailya
ilyailya approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor
@ilyailya ilyailya left a comment

Choose a reason for hiding this comment

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

LGTM!

@jez jez merged commit a3e2337 into master Oct 24, 2022
@jez jez deleted the jez-no-check-block-arg branch October 24, 2022 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilyailya ilyailya ilyailya approved these changes

@froydnj froydnj Awaiting requested review from froydnj froydnj is a code owner automatically assigned from sorbet/core-reviewers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jez @ilyailya
0