Two sorbet-runtime call-validation optimizations #4158

djudd-stripe · 2021-04-18T17:39:34Z

Two changes:

Deprecate T::Profile, and rip out the sampled timing code from call validation.
Add a new semi-fast path for call validation with up to four positional arguments and no keyword arguments or other special cases, very similar to the existing fast path except without the restriction that types must be T::Types::Simple. (~~Also method-vs-procedure is left for a runtime check, for now, to avoid blowing up code size and because one extra branch didn't seem like a big deal relative to the rest.~~ edit: when merging the generator code, it was simpler to just leave this distinction in for both paths.)

These are really related only in intent; I actually implemented T::Profile support in the new fast-ish path before ripping it out. So I'm happy to split into separate PRs if folks prefer.

Motivation

I don't believe this timer measures anything useful. It doesn't account for a significant proportion of runtime typechecking overhead, probably the majority of overhead, because (a) it doesn't capture the impact of adding the define_method stack frame itself, and (b) it doesn't capture any impact of T.let, T.must, T.unsafe etc. A tool like StackProf will give a much better picture, and (if configured well) with much lower measurement overhead. Microbenchmarks show >20% speedup on single-argument methods.
I don't have hard statistics but my impression is that a large number of relatively-simple methods fail to qualify for the existing fast path because they use something like T::Boolean, T.nilable, or T.untyped, even as just one argument among otherwise simple ones. Microbenchmarks show a >40% speedup on single-nilable-argument methods.

bundle exec rake bench:typecheck results for 2.7, slightly abridged, on:

master:

T::Types::Simple#valid?: 41.252 ns
T::Types::Union#valid?: 139.641 ns
T.let(..., Integer): 428.153 ns
sig {params(x: Integer).void}: 337.683 ns
T.let(..., T.nilable(Integer)): 1.036 μs
sig {params(x: T.nilable(Integer)).void}: 862.766 ns
T.let(..., Example): 409.354 ns
sig {params(x: Example).void}: 307.338 ns
T.let(..., T.nilable(Example)): 1.043 μs
sig {params(x: T.nilable(Example)).void}: 875.392 ns
sig {params(s: Symbol, x: Integer, y: Integer).void} (with kwargs): 1.504 μs

just the new fast path:

T::Types::Simple#valid?: 42.637 ns
T::Types::Union#valid?: 142.595 ns
T.let(..., Integer): 389.78 ns
sig {params(x: Integer).void}: 330.284 ns
T.let(..., T.nilable(Integer)): 1.004 μs
sig {params(x: T.nilable(Integer)).void}: 492.045 ns
T.let(..., Example): 396.391 ns
sig {params(x: Example).void}: 313.137 ns
T.let(..., T.nilable(Example)): 1.01 μs
sig {params(x: T.nilable(Example)).void}: 500.454 ns
sig {params(s: Symbol, x: Integer, y: Integer).void} (with kwargs): 1.507 μs

with both changes:

T::Types::Simple#valid?: 40.547 ns
T::Types::Union#valid?: 161.091 ns
T.let(..., Integer): 405.242 ns
sig {params(x: Integer).void}: 215.672 ns
T.let(..., T.nilable(Integer)): 1.038 μs
sig {params(x: T.nilable(Integer)).void}: 377.882 ns
T.let(..., Example): 393.038 ns
sig {params(x: Example).void}: 207.139 ns
T.let(..., T.nilable(Example)): 1.078 μs
sig {params(x: T.nilable(Example)).void}: 394.03 ns
sig {params(s: Symbol, x: Integer, y: Integer).void} (with kwargs): 1.424 μs

(Only the changes in the sig results could be meaningful, but I've left the others in as points of comparison & indicators of the noise level.)

Test plan

Relying on existing tests

…ex types

jez · 2021-04-18T18:29:08Z

Do the api method tests use T::Profile right now in Stripe codebase? Is the plan to delete or change those tests?

djudd-stripe · 2021-04-18T19:54:09Z

@jez they don't. If you're talking about the no-hot-runtime-checking tests, they use a custom mechanism so they can report more information about which methods are called. T::Profile is only used in a couple of production log-lines and I don't believe it provides real value there: https://livegrep.corp.stripe.com/search/stripe?q=T%3A%3AProfile%20repo%3Apay-server&fold_case=auto&regex=false&context=true

(It was maybe a bit more valuable before the introduction of the no-hot-runtime-checking tests, and when recursive checking was the default, so it was easier to introduce a regression and having a number right in the canonical log line could be valuable. I don't think this really applies anymore.)

jez · 2021-04-19T20:34:34Z

gems/sorbet-runtime/lib/types/profile.rb

@@ -1,30 +1,25 @@
 # typed: true
 # frozen_string_literal: true

+# Deprecated, kept only for partial backwards compatibility


When do you plan to remove this completely?

Not sure. Could potentially remove it completely now if you want; I'll have to do some advance work in pay-server anyway to keep tests passing.

yeah i don't think we every documented or publicized this much, so i think it's safe to just remove the ~5 usages of it in Stripe's codebase pre-emptively and then land this PR.

jez · 2021-04-19T20:34:48Z

T::Profile is only used in a couple of production log-lines and I don't believe it provides real value there

Gotcha, so it seems like it's basically just populating some log lines, and since you've made a case that those log lines are not even really accurate, that it's not worth keeping these things around.

jez · 2021-04-19T20:35:41Z

gems/sorbet-runtime/tools/generate_call_validation.cc

-    fmt::print("      # This block is called for every `sig`. It's critical to keep it fast and\n"
-               "      # reduce number of allocations that happen here.\n"


ooc why have these comments been dropped?

It's not literally true - none of these individual methods is called for every sig anymore - and it didn't seem like it was adding enough value to be worth trying to reword.

jez · 2021-04-19T20:43:45Z

gems/sorbet-runtime/tools/generate_call_validation.cc

+               "          CallValidation.report_error(\n"
+               "            method_sig,\n"
+               "            message,\n"
+               "            'Return value',\n"
+               "            nil,\n"
+               "            return_type,\n"
+               "            return_value,\n"
+               "            caller_offset: -1\n"
+               "          )\n"
+               "        end\n"


The other two (methods, procedures) take almost the same codepath but then branch on validatorKind in a few places.

It seems like the "medium" path is pretty similar to the fast path, differing only by

the names of the individual methods

the condition used to check the types

Do you think it's worth trying to share more code between the different kinds of validators?

Or at least factor out some of the shared code?

The handling of return_type is different too, but that doesn't affect your point much.

I agree that more code could be shared; I started off with a separate method because I wasn't sure how much would end up being shared, but it ended up being most of it.

I think merging might make things less readable - I already find the switches on ValidatorKind a bit hard to read -but it would make it less likely that some difference could be introduced accidentally, so maybe it's worth it. WDYT?

i tend to think that in terms of "hard to read," if you're trying to change this code you're going to be looking at the generated code, imagining how you'd like the generated code to change, and back-solving for a diff to the the generator (i.e., the C++ code is not read in a vacuum, it is read in tandem with the generated code).

Given that, I place more value on the second part (introducing an accidental divergence).

So yeah, if you don't feel strongly one way or another i'd love to share more of the repetitive code between these two.

but it ended up being most of it

for what it's worth, this is basically the same process and realization that I had when I wrote it initially (it started out as two, and then I realized that so little was different)

jez

Change looks mostly good to me. I have one question about rollout / deprecation strategy, and one question about how we can share a little more code.

Happy to approve after having a chat about those things.

djudd-stripe · 2021-04-20T15:39:31Z

@jez updated to remove T::Profile fully and to share more call-validation-generator code. The diff does look much cleaner with the latter change. The Stripe codebase is also now prepped for this (PR just merged).

djudd-stripe added 3 commits April 17, 2021 20:33

Add medium-fast path for call validation with positional args & compl…

45f7b3c

…ex types

Remove self-timing from call validation

0de0ea9

Rip out remaining T::Profile internals

a0875ce

djudd-stripe requested a review from a team as a code owner April 18, 2021 17:39

djudd-stripe requested review from elliottt and removed request for a team April 18, 2021 17:39

autoformat c++

f14ab88

djudd-stripe added 2 commits April 19, 2021 10:51

Fix method validation allocation counting test

3552cc8

Remove accidentally-committed .ruby-version

5297632

jez reviewed Apr 19, 2021

View reviewed changes

elliottt removed their request for review April 19, 2021 20:38

jez reviewed Apr 19, 2021

View reviewed changes

djudd-stripe added 2 commits April 19, 2021 16:33

Fully remove T::Profile

ea882be

DRY up call validation generator code

ede071e

djudd-stripe force-pushed the djudd/semi-fast-path branch from 839173f to ede071e Compare April 19, 2021 23:42

jez approved these changes Apr 27, 2021

View reviewed changes

djudd-stripe merged commit 45d99dc into master Apr 27, 2021

djudd-stripe deleted the djudd/semi-fast-path branch April 27, 2021 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Two sorbet-runtime call-validation optimizations #4158

Two sorbet-runtime call-validation optimizations #4158

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		fmt::print(" # This block is called for every `sig`. It's critical to keep it fast and\n"
		" # reduce number of allocations that happen here.\n"

Two sorbet-runtime call-validation optimizations #4158

Two sorbet-runtime call-validation optimizations #4158

Uh oh!

Conversation

Uh oh!

Motivation

Test plan

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!