Skip to content
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.

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

Use match? in FactoryBot#aliases_for to save allocations #1457

Merged
merged 1 commit into from
Sep 10, 2021

Conversation

Aqualon
Copy link
Contributor

@Aqualon Aqualon commented Dec 29, 2020

FactoryBot relies on ruby >= 2.5, so we can use Regexp#match? that does not allocate MatchData

Allocations measured with heap-profiler gem in an internal test suite with 7497 examples and 149470 calls to aliases_for:

allocated memory by file

59.79 MB  factory_bot-6.1.0/lib/factory_bot/aliases.rb
38.27 MB  factory_bot-6.1.0/lib/factory_bot/aliases.rb

allocated objects by file

896880  factory_bot-6.1.0/lib/factory_bot/aliases.rb
597940  factory_bot-6.1.0/lib/factory_bot/aliases.rb

The remaining allocations are mainly caused by

attribute.to_s.sub(pattern, replace).to_sym

In the test suite I used, I noticed that attribute values are repeated quite often. For the 149470 calls I just got 16 unique attribute values. So a simple caching mechanism prevents most of the allocations:

  @aliases_for_cache = {}

  def self.aliases_for(attribute)
    @aliases_for_cache[attribute] ||=
      aliases.map { |(pattern, replace)|
        if pattern.match?(attribute)
          attribute.to_s.sub(pattern, replace).to_sym
        end
      }.compact << attribute
  end

In another test suite I got 56 unique attribute values for 228979 calls to aliases_for. I don't know if this is representative for other users of FactoryBot, but I could prepare a second PR with the cache idea.

FactoryBot relies on ruby >= 2.5, so we can use Regexp#match? that does not allocate MatchData

Allocations measured with heap-profiler gem in an internal test suite with 7497 examples and 149470 calls to aliases_for:

allocated memory by file
-----------------------------------
    59.79 MB  factory_bot-6.1.0/lib/factory_bot/aliases.rb
    38.27 MB  factory_bot-6.1.0/lib/factory_bot/aliases.rb

allocated objects by file
-----------------------------------
    896880  factory_bot-6.1.0/lib/factory_bot/aliases.rb
    597940  factory_bot-6.1.0/lib/factory_bot/aliases.rb
@@ -10,7 +10,7 @@ class << self

def self.aliases_for(attribute)
aliases.map { |(pattern, replace)|
if pattern.match(attribute.to_s)
Copy link

Choose a reason for hiding this comment

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

Why are we removing to_s? Do we know why we were calling it here in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about this part either, I tried it with symbol and string attributes and it both worked. But maybe there could be scenarios where attribute is another ruby object, that implements to_s? Don't know enough details about factory_bot there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tracing back through the callers it seems like attribute here is always a symbol, and Regexp#match seems to work fine with a symbol (maybe that wasn't always true?), so change seems good to me.

@MatheusRich
Copy link

How can we move this forward? Just adding .to_s back would make this ready to go?

@composerinteralia composerinteralia merged commit 893eb67 into thoughtbot:master Sep 10, 2021
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