Skip to content

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Feb 10, 2023

Motivation

I wanted to get a more complete understanding of the LSP's performance on a long running session, so I added the following to server.rb to generate profile data.

def start
  # ...
  when "initialized"
    StackProf.start(mode: :cpu, raw: true)

  when "shutdown"
    StackProf.stop
    data = StackProf.results
    File.write("profile.json", JSON.pretty_generate(data))
  # ...
end

I edited and saved files for 2 minutes to triggered as many requests as possible. To my surprise, of the 120 seconds we spent 30 of them just initializing T::Struct objects (specifically, running the construct with defaults thing and performing runtime typechecks with is_a?).

Other than the desire to have very compact class definitions, we really have no other requirement for using T::Struct and it doesn't seem like a good trade off for the performance penalty.

Using benchmark IPS to check the difference on a single request, not counting objects created for queueing, there's a 7% performance difference.

Warming up --------------------------------------
                 new    15.350k i/100ms
Calculating -------------------------------------
                 new    154.727k (± 1.1%) i/s -    782.850k in   5.060214s

Comparison:
                 new:   154727.1 i/s
                 old:   144077.7 i/s - 1.07x  (± 0.00) slower

This also benefits YJIT, which in my local machine goes from 1.24 -> 1.32 interp / yjit ratio.

Implementation

Switched all T::Struct to regular classes.

@vinistock vinistock requested a review from a team as a code owner February 10, 2023 20:02
@vinistock vinistock self-assigned this Feb 10, 2023
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

Can we figure out how to improve T::Struct as well? Maybe we should always be shipping our own simplified version of T::Struct at runtime, so that we never have to do this work, nor worry about performance.

Copy link
Contributor

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

It seems there are some improvements we could be shipping Sorbet's upstream either on T::Struct itself or its runtime configuration.

@vinistock vinistock force-pushed the vs/remove_tstruct_usage branch from 4653da2 to 0ccb10c Compare February 13, 2023 21:00
@vinistock
Copy link
Member Author

Indeed, there might be an opportunity. I think @bitwise-aiden has done some exploration on the performance of instantiating T::Struct.

@bitwise-aiden
Copy link
Contributor

Can we figure out how to improve T::Struct as well? Maybe we should always be shipping our own simplified version of T::Struct at runtime, so that we never have to do this work, nor worry about performance.

+1 to this. Off the back of the investigation into the constructor, I think that it might be worth replacing T::Struct for PORO at runtime. I will have a bit of a look into that this week before going on break.

@vinistock vinistock merged commit 9cf65ec into main Feb 14, 2023
@vinistock vinistock deleted the vs/remove_tstruct_usage branch February 14, 2023 15:43
@shopify-shipit shopify-shipit bot temporarily deployed to production February 15, 2023 20:50 Inactive
vinistock pushed a commit that referenced this pull request Feb 28, 2024
…-2.8.7

Bump prettier from 2.8.6 to 2.8.7
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.

4 participants