Speed up by kddnewton · Pull Request #69 · Shopify/smart_todo

kddnewton · 2023-10-27T18:27:51Z

👋 Hi! I heard this could benefit from some improved performance, and also that it used ripper. I'm on a mission to remove ripper from everywhere. So, I took a look. I did a couple of things to improve performance, namely:

I swapped out ripper for prism. It requires less knowledge of the AST, is easier to maintain, and is faster if you're just parsing comments.
I changed Events from a module to a class. Previously, every event on every todo lined up to a method call to the Events module. That module would then instantiate whatever data it needed to do its job. But none of this data could be shared between todos. So it was discarded when the method call returned. Now instead we cache the data we want to keep around (like rubygems or github clients).
I inlined a couple of method calls, removed some dynamic dispatch, and made sure the code was a little more shape friendly by initializing instance variables in initializers.
I made the dispatches parallel so that when we're actually hitting slack it will switch threads to make multiple requests at once.

There are still quite a few improvements I think could be put in place, but this seemed like a good place to stop. Those would be:

Grouping events before they are checked, then checking them as a single corpus. For example, if you grouped all of the date events together, you could sort them and then find the index of the first one to match, then take the remaining. For github-related events, you could create a single HTTP connection instead of one per event.
Right now a different HTTP connection is made to slack on every dispatch.
Replace prism parsing the body of the todo with stringscanner. I imagine this would be faster, and you would get better error messages.

Overall, with these improvements in place, the time to parse Shopify's monolith on my computer went from 1:26.26 to 22.789:

CI=1 bin/profile ../shopify  65.70s user 2.71s system 79% cpu 1:26.26 total
CI=1 bin/profile ../shopify  3.86s user 2.30s system 27% cpu 22.789 total

A couple of things to note:

Right now it's pointing to a GitHub version of prism because I created a new API for this. I'll release the new version soon.
I removed the rexml dependency because it was never used.

Sorry about the size of this PR. I saw a bunch of stuff so I just kept going. Happy to trim this back to individual small commits if you'd prefer. I just figured I'd get this in front of eyes now and then we can reevaluate the scope of it.

Gemfile

rafaelfranca · 2023-10-27T18:42:19Z

lib/smart_todo/events.rb

+    private
+
+    def now
+      @now ||= Time.now


Why aren't we just setting this on the initialize?

Same answer - if you never get a on: date then we don't need to create this object.

rafaelfranca · 2023-10-27T18:42:37Z

lib/smart_todo/events.rb

+    def rubygems_client
+      @rubygems_client ||= Net::HTTP.new("rubygems.org", Net::HTTP.https_default_port).tap do |client|
+        client.use_ssl = true
+      end
+    end
+
+    def github_client
+      @github_client ||= Net::HTTP.new("api.github.com", Net::HTTP.https_default_port).tap do |client|
+        client.use_ssl = true
+      end
+    end
+
+    def installed_ruby_version
+      @installed_ruby_version ||= Gem::Version.new(RUBY_VERSION)


Why aren't we setting those in the initialize?

We're lazily setting these up because if you don't find any todos that have a gem version, there's no need to instantiate these objects. Basically it mirrors the previous behavior of lazily creating objects based on the todos you find.

rafaelfranca · 2023-10-27T18:43:03Z

lib/smart_todo/events/date.rb

        # @return [String, false]
-        def met?(on_date)
-          if Time.now >= Time.parse(on_date)
+        def met?(on_date, now = Time.now)


Do we even use this method without the second argument?

We don't, I just wasn't sure how much you wanted to break the existing APIs. Happy to make this required if you're prefer.

It is fine to break API, all those classes are private API.

Nice, in this case I've inlined the classes.

rafaelfranca · 2023-10-27T18:43:22Z

lib/smart_todo/events/gem_bump.rb

      # @example Expecting a version in the 5.x.x series
      #   GemBump.new('rails', ['> 5.2', '< 6'])
-      def initialize(gem_name, requirements)
+      def initialize(gem_name, requirements, spec_set = Bundler.load.specs)


Do we use this class without the last argument?

Nope, removed the class wholesale.

rafaelfranca · 2023-10-27T18:43:37Z

lib/smart_todo/events/gem_release.rb

      # @example Expecting a version in the 5.x.x series
      #   GemRelease.new('rails', ['> 5.2', '< 6'])
-      def initialize(gem_name, requirements)
+      def initialize(gem_name, requirements, client = nil)


rafaelfranca · 2023-10-27T18:43:42Z

lib/smart_todo/events/issue_close.rb

      # @param repo [String]
      # @param pr_number [String, Integer]
-      def initialize(organization, repo, pr_number, type:)
+      def initialize(organization, repo, pr_number, client = nil, type:)


rafaelfranca · 2023-10-27T18:43:49Z

lib/smart_todo/events/ruby_version.rb

-      def initialize(requirements)
+      attr_reader :installed_ruby_version
+
+      def initialize(requirements, installed_ruby_version = Gem::Version.new(RUBY_VERSION))


kddnewton · 2023-11-02T17:52:32Z

Just one sec - I released the new version of Prism so I'll make it point to rubygems.org

kddnewton · 2023-11-03T15:37:42Z

@rafaelfranca this is good to go now. Should I merge?

rafaelfranca · 2023-11-08T22:03:19Z

Yes.

kddnewton force-pushed the speed branch from aaf5d29 to 107114c Compare October 27, 2023 18:29

rafaelfranca reviewed Oct 27, 2023

View reviewed changes

Gemfile Outdated Show resolved Hide resolved

rafaelfranca reviewed Oct 27, 2023

View reviewed changes

kddnewton force-pushed the speed branch from 107114c to 1e22d49 Compare October 30, 2023 14:43

rafaelfranca approved these changes Nov 2, 2023

View reviewed changes

Speed

6aa2c2a

kddnewton force-pushed the speed branch from 1e22d49 to 6aa2c2a Compare November 3, 2023 15:37

kddnewton merged commit 6c07c84 into main Nov 8, 2023

kddnewton deleted the speed branch November 8, 2023 22:16

shopify-shipit bot temporarily deployed to rubygems December 8, 2023 17:10 Inactive

Conversation

kddnewton commented Oct 27, 2023

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kddnewton commented Nov 2, 2023

Uh oh!

kddnewton commented Nov 3, 2023

Uh oh!

rafaelfranca commented Nov 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants