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

Profile: Rework state for text while taking notes to remove lag #2415

merged 3 commits into from Feb 15, 2019


Copy link

@kevinrobinson kevinrobinson commented Feb 15, 2019

Who is this PR for?

educators on older computers, or on IE11

What problem does this PR fix?

This resolves a performance issue in taking notes. This has only been visible in developer recently, but I assumed that on my old laptop it was mostly from battery issues and the OS throttling the CPU. But this came up for an educator today, so I looked into it more.

With my laptop at full power, there's no lag impacting the UX while typing. But I can simulate the lag in Chrome by throttling CPU by a factor of 4x or 6x, although never enough to freeze things altogether or crash the tab. On a battery-impaired laptop where the OS is throttling the CPU severely, I can reliably get up 10 second (!) latency on keystrokes in Chrome, or can crash the tab. This can lead to crashing Firefox altogether.

So I think the impact of this is limited to folks like me with degraded laptop batteries, and some educators working on older computers with IE11, where there is very little compute available.

CPU throttled from battery, examples of slow script warnings

screen shot 2019-02-15 at 9 15 38 am

screen shot 2019-02-15 at 8 59 10 am

screen shot 2019-02-15 at 9 03 25 am

It's surprising to me that this is a problem, since this code hasn't changed in a long time. I suspected it might be related to the React update in #2397 but performance is actually worse on commit 6f8d2d8. So my understanding is that this is probably only impacting low-end CPUs on IE11.

Using the JS and React profiler, it's clear this is in JS script executing, that most of it is inside React and that it's related to the state tracking text as the user types, and it being lifted up and causing renders of unrelated bits of the page.

What does this PR do?

Since this is causing an immediate problem, and since we were going to replace this part of the UI altogether in the next week or two, this PR takes a shortcut and removing lifting up the state to avoid the renders, rather than optimizing code we're about to replace.

The state was lifted previously to preserve in-progress notes as the user navigates to other tabs. This is uncommon, and will be reworked anyway with autosaving notes. So as a guardrail in the meantime, this only hoists that a new note is in-progress, and then pops a confirm box up to warn the user they'll lose it if they navigate away.

profiling caveat

Keep in mind these are on a laptop with a busted old battery, so it's usually doing CPU throttling :) This has a really severe impact on performance.

screen shot 2019-02-15 at 10 54 43 am

before, profiling with full battery

screen shot 2019-02-15 at 11 44 52 am

screen shot 2019-02-15 at 11 45 17 am

before, profiling with impaired battery

screen shot 2019-02-15 at 10 41 50 am

screen shot 2019-02-15 at 10 42 11 am

after, profiling with impaired battery

screen shot 2019-02-15 at 11 01 34 am

screen shot 2019-02-15 at 11 05 04 am

Screenshot (if adding a client-side feature)

ux warning on clicking another tab while taking notes

screen shot 2019-02-15 at 10 47 43 am


Which features or pages does this PR touch?

  • Student Profile

Does this PR use tests to help verify we can deploy these changes quickly and confidently?

  • Improved automated test coverage
  • Manual testing made more sense here

Copy link
Contributor Author


@kevinrobinson kevinrobinson merged commit c0cefc1 into master Feb 15, 2019
@kevinrobinson kevinrobinson deleted the feature/remove-note-in-progress branch February 15, 2019 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

1 participant