Skip to content

Conversation

@bkeepers
Copy link
Collaborator

I noticed #features was calling Gate.all.map(&:key).to_set to get the list of features, which will pull a lot of data over the wire to get the list of features. This replaces it with Gate.distinct.pluck(:key).to_set. Thankfully, #features is not used in our hot code path internally (get_all with memoization is used when checking #enabled?), but this could have some impact for those with large actor sets.

$ ruby benchmarks/active_record_ips.rb

# Before:
            features      6.453k (± 8.3%) i/s -     32.604k in   5.088062s
# After:
            features     10.130k (± 8.6%) i/s -     50.661k in   5.038290s

This benchmark uses an sqlite in-memory database, so the results should be even more dramatic over a network connection.

@bkeepers bkeepers requested a review from jnunemaker February 17, 2024 13:04
@bkeepers bkeepers changed the title Microoptimization for #features on ActiveRecord adapter Micro optimization for #features on ActiveRecord adapter Feb 17, 2024
Copy link
Collaborator

@jnunemaker jnunemaker left a comment

Choose a reason for hiding this comment

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

this could have some impact for those with large actor sets.

You said Gate and actor sets a few times which confused me but I think you meant Feature and large feature sets.

This is great! Good find.

@bkeepers
Copy link
Collaborator Author

You said Gate and actor sets a few times which confused me but I think you meant Feature and large feature sets.

I did actually mean gates and actor sets. Since this is querying the gates table, it loads all gate data to just get the distinct feature names. So if you enable a single feature for a lot of actors, then calling Flipper.features will pull all the actor data over the wire just to get the feature list.

@bkeepers bkeepers merged commit b36d97e into main Feb 17, 2024
@bkeepers bkeepers deleted the ar-optimize branch February 17, 2024 15:50
@jnunemaker
Copy link
Collaborator

Ok. I saw @feature_class everywhere not @gate_class so I guess I don't understand where gates were even being queried but that's ok. Regardless this is a good improvement!

@bkeepers
Copy link
Collaborator Author

🤦‍♂️ wow, I must be low on sleep or something. You’re absolutely right. I was thinking it was gates. Now I’m confused where the improvement is coming from.

Must just be large feature sets like you said.

@jnunemaker
Copy link
Collaborator

🤣. I was definitely feeling crazy when you said you meant gate. Haha.

This would definitely improve getting features as plucking a column is way faster than pulling all feature data across wire and instantiating AR objects. It's a good change especially for lots of features.

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.

3 participants