Use Sorbet cache (if enabled)

[amomchilov]

Motivation

If a repo has enabled the Sorbet cache, we should use it.

Implementation

Builds upon the SorbetCache #2568.

Performance

Shaves off around ~3s on the Shopify core monolith.

The cache stores the desugared ASTs, after the parsing, RBS rewriting, and desugaring is done. If there's a cache hit, no parsing happens, so that's why you see the legacy and Prism parsers have the same time. So this mitigates some of the benefit of #2567, but Prism is still helpful when there's a cache miss.

hyperfine --warmup 5 --runs 20 \
  -n "prism, with cache"  '~/sorbet-master --no-config --cache-dir=tmp/sorbet_cache --parser=prism    --print=symbol-table-json --quiet --stop-after=namer .' \
  -n "legacy, with cache" '~/sorbet-master --no-config --cache-dir=tmp/sorbet_cache --parser=original --print=symbol-table-json --quiet --stop-after=namer .' \
  -n "legacy, no cache"   '~/sorbet-master --no-config --cache-dir=                 --parser=original --print=symbol-table-json --quiet --stop-after=namer .' \

  -n "prism, no cache"    '~/sorbet-master --no-config --cache-dir=                 --parser=prism    --print=symbol-table-json --quiet --stop-after=namer .'

Benchmark 1: "prism, with cache"
  Time (mean ± σ):     24.932 s ±  0.687 s    [User: 23.678 s, System: 18.433 s]
  Range (min … max):   23.812 s … 26.870 s    20 runs
  
Benchmark 2: "legacy, with cache"
  Time (mean ± σ):     25.031 s ±  0.354 s    [User: 23.550 s, System: 18.331 s]
  Range (min … max):   24.518 s … 25.917 s    20 runs

Benchmark 3: "prism, no cache"
  Time (mean ± σ):     26.880 s ±  0.494 s    [User: 44.113 s, System: 18.467 s]
  Range (min … max):   25.809 s … 27.619 s    20 runs
  
Benchmark 4: "legacy, no cache"
  Time (mean ± σ):     28.100 s ±  0.795 s    [User: 64.862 s, System: 18.303 s]
  Range (min … max):   27.020 s … 29.405 s    20 runs


Summary
  "prism, with cache" ran
    1.00 ± 0.03 times faster than "legacy, with cache"
    1.08 ± 0.04 times faster than "prism, no cache"
    1.13 ± 0.04 times faster than "legacy, no cache"

Tests

Added.

[amomchilov]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• Use RBS in Sorbet calls (if enabled) #2569
• Use Sorbet cache (if enabled) #2568 👈 (View in Graphite)
• Use Prism in Sorbet calls (if enabled) #2567
• Extract sorbet_syntax_check! #2572
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[KaanOzkan]

I think this would be more readable outside of new, was there a specific reason to have it here?

Also should this use shellescape?

[amomchilov]

Sure, I extracted local variables

Also should this use shellescape?

I don't think so. The C++ code consumes these values directly, not mediated by a shell.

[Morriar]

Sure, I extracted local variables

Did you?

Are we missing a push?

[KaanOzkan]

Looks like it. LGTM after that.