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

Feature flags can be costly when read zillions of times #18004

Closed
jryans opened this issue Jul 14, 2021 · 6 comments
Closed

Feature flags can be costly when read zillions of times #18004

jryans opened this issue Jul 14, 2021 · 6 comments

Comments

@jryans
Copy link
Collaborator

jryans commented Jul 14, 2021

Some feature flags (such as the one for Spaces) are checked in hot code paths, such as isRoomVisible. Every time we check a feature flag, we read it from localStorage. 😱

We should probably cache those feature flags in memory instead.

@robintown
Copy link
Member

I don't know the full reasons why toggling the spaces beta requires the client to restart, but perhaps putting settings watchers on feature_spaces would have the added benefit of allowing you to toggle it without a restart?

@t3chguy
Copy link
Member

t3chguy commented Jul 15, 2021

I don't know the full reasons why toggling the spaces beta requires the client to restart, but perhaps putting settings watchers on feature_spaces would have the added benefit of allowing you to toggle it without a restart?

The space store isn't running with the flag off so it'll have missed many events and it's easier maintaining the assumption that it'll be given all events it needs and not miss any.

Also so the groups store isn't holding onto things
And you'd have to listen to not only feature_spaces but also the settings for communities, custom tags and communities V2 as all those get disabled when you enable spaces

@turt2live
Copy link
Member

also the RoomListStore and friends need to be told to recalculate the world, which can be expensive. By the time that you've updated all the stores, the client will have locked up enough where a restart/refresh is less disruptive.

@t3chguy
Copy link
Member

t3chguy commented Jul 15, 2021

But for the Spaces beta specifically we can lean into that and read it once and know it won't change in this lifetime - I will do so

@robintown
Copy link
Member

Ah, then nevermind

@t3chguy
Copy link
Member

t3chguy commented Jul 28, 2023

Fixed by matrix-org/matrix-react-sdk#8366

@t3chguy t3chguy closed this as completed Jul 28, 2023
@t3chguy t3chguy self-assigned this Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants