jkomoros / card-web

The web app behind thecompendium.cards
Apache License 2.0
46 stars 8 forks source link

When editing cards in production, typing has gotten slow #564

Open jkomoros opened 2 years ago

jkomoros commented 2 years ago

Looks like overshadowedDiffChanges is run on every keystroke, which also runs normalizeBodyHTML

jkomoros commented 2 years ago

It's called because card-editor selects selectEditingCardHasUnsavedChanges, selectOvershadowedUnderlyingCardChangesDiffDescription, and both of those call cardDiffGeneration, which also calls normalizeBodyHTML

jkomoros commented 2 years ago

Maybe just make those two properties delayed set in setState?

jkomoros commented 2 years ago

In production it's way, way slower. the cardDiffGeneration is definitely part of it, but also a lot of objectEquality with a lot of major and minor gc

jkomoros commented 2 years ago

It's slower in the main version of Chrome, not Canary/Dev... likely because I have so many active/expensive tabs in the main Chrome that are conflicting with some kind of shared resource? Canary doesn't seem to really hit it that hard.

The ObjectEquailty as being expensive doens't show up in the non-compiled main Chrome for some reason.

jkomoros commented 2 years ago

Node use does steadily climb, which likely comes from generateCardDiff, since it normalizes the underlying text

jkomoros commented 2 years ago

After 1be94690a5aab6c2527d801ba7ec5f87fd362c5f, the trace now just shows that the update/render is the most expensive thing, consistently making the whole thing miss the frame boundary by 50% or more and causing jank.

jkomoros commented 2 years ago

It's still a bit slow, but non deterministically. It's spending most of the time in render(). It seems to have something to do with how much contention there is with other live tabs in the browser?

jkomoros commented 2 years ago

OK as of 43f62cfbb0d80d56b4ccedd3f07c6ab6aad157ef we need a new strategy, because editingNormalizedCard only updates when text fields that need nlp extraction changes.

Caching/memoziation doesn't work because the text field will be changing often in the main update codepath.

The only viable option is generateCardDiff gets skipTextNormalization option. Most uses of it skip normalization, except for generateFinalCardDiff, which does need to normalize.

The one question is EDITING_UPDATE_UNDERLYING_CARD updater in reducers/data, which figures out the diff to apply on top of the underlyin gcard. I THINK that's OK to change because we don't do anything special with intra-field diffing (yet, see #503 ), so if body has been touched the whole thing is blown away in the diff anyway.

jkomoros commented 2 years ago

Note that even generateFinalCardDiff might not need to normalize HTML because it's already normalized earlier in the finisher? Or is that the only place that the final text normalization is done? In any case, it's a not-very-expensive thing happening only at editing commit so it's fine

jkomoros commented 2 years ago

580 #581 #585 are all related