Improve performance of enabling/disabling extensions

Created by: tjkandala

Problem

There is noticeable lag when enabling or disabling an extension on the extensions registry. During the period between click and confirmation, there is no loading state/visual feedback, so I often "rage clicked" the toggle. This would lead to the extension toggling itself on and off ~2 seconds later.

I originally assumed that this was due to network latency, but far more time was spent scripting than waiting for the backend.

development environment (2.0s and 1.1s to render toggles after getting latest settings from backend)

It isn't as slow in the production build, but it is still well over the recommended 100ms budget for user actions. Combined with waiting for two serial requests, both the mutation and the query, toggling an extension regularly takes over half a second.

sourcegraph.com/extensions (331ms and 457ms to render toggles after getting latest settings from backend)

Cause

Lifecycle of an extension toggle

After the mutating settings and fetching the latest settings, we push the settings cascade to to subscribers of platformContext.settings. There are 5 separate subscriptions to platformSettings in the root component, each of which calls setState on each emission. The settings cascade is passed all the way down the tree.

Short-term fix

Changes:

• Add enabled prop to ExtensionCard and evaluate isExtensionEnabled in ExtensionsList.
• Refactor ExtensionCard into a functional component. This was a PureComponent before, but because settingsCascade is an entirely new object after fetchViewerSettings, every ExtensionCard renders even if its toggle state didn't change.
• Wrap ExtensionCard with React.memo and provide custom comparator to bail out of render if the toggle state hasn't changed, which should be true for N - 1 extensions. Currently, all ExtensionCards render on every settings change, and the enabled state isn't evaluated until ExtensionToggle.
• Memoize the icon URL with React.useMemo. Constructing the URL seems to be the most expensive operation here, and it is currently happening 138 times, once for each extension, per render.
• Use extension id instead of array index as key in ExtensionsList (to enable better filtering on the client if we are given more metadata, namely categories)

The result: 297ms and 255ms to render toggles after getting latest settings from server. It's now faster in the developer environment than the current production environment.

Longer-term proposals

The above changes significantly improve the performance of the extensions registry, but they are still mostly band-aids over overarching structural issues. These are not concrete proposals, as I am continuing to learn about the Sourcegraph web app architecture, but are things that I think we should consider as a team.

Medium-term: Combine the 5 subscriptions to platformContext.settings into one. I found significant improvement just by reducing it to 3 subscriptions, but reverted it because it was harder to read. Some clever refactoring will be necessary to consolidate all of the subscriptions. I like the way that globbingEnabledFromSettings is implemented. We could define helper functions like that to derive slices state from a single subscription. This is low-hanging fruit that will easily improve performance of the extensions registry

Long-term: Consider introducing an external state management library. The use of RxJS streams has enabled communication from leaf nodes like Toggle to the application root node, which is one of the common features of state management libraries in React, but this solution may be inadequate for several reasons.

• Low granularity. We pass down emissions of the entirely new settings state through to the leaves. With a library like Recoil or Redux, it is easier to both read small slices of and make small changes to a large tree of configuration objects at any point in the component tree. Essentially, avoiding prop drilling and unnecessary renders due to referential inequality of unchanged (by value) objects.
• Related to the point above, we lose context of the motivating event; why has state changed? This may make debugging harder than it has to be.
• PureComponent/Memo aren't free, so we should create a foundation/pit of success that doesn't necessitate their use or harm performance if we forget to use them. A developer should not be at risk of causing 2 second renders from an innocent-looking component like ExtensionCard. EDIT: Upon further consideration, we can make these changes w/ RxJS alone. Using a library would be nice for documentation + ecosystem, though.

Notes/questions

• Is there some type of extension dependency system that I'm missing? For example, if sqs/word-count depended on sourcegraph/typescript, disabling sourcegraph/typescript would disable sqs/word-count as well. If not, we can definitely simplify the code further.
• TODO: integration tests for extensions registry
• ExtensionToggle's title if the highestPrecedenceSubject is "default settings" is messed up. I'll look into it later.