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.

Sign up for GitHub

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

Jump to bottom

[Performance] Strangely high CPU usage in Chrome when editing a display layout #1603

Closed
ghost opened this issue Jun 1, 2017 · 8 comments
Closed

[Performance] Strangely high CPU usage in Chrome when editing a display layout #1603

ghost opened this issue Jun 1, 2017 · 8 comments
Assignees
@charlesh88
Milestone
Tiptree

Comments

@ghost
Copy link

ghost commented Jun 1, 2017 •
edited by ghost

See this gif here: http://imgur.com/QAMfBoc
As soon as I start editing an empty display layout, the CPU usage in Chrome 58.0.3029.110 (64-bit) (latest at the time of writing) goes nuts and my laptop turns into a wind turbine.

Normally I wouldn't mind this that much, but the UI itself also ends up being very sluggish. For example, scrolling in the save as dialog shown in the gif feels unpleasant to say the least.

Quitting the edit process results in smooth performance once again.

It's been a while since I tried out a build of OpenMCT, a few months ago (around January) I'm almost 100% sure I used the same laptop for it and the bad Chrome behavior did not exist. I'm not sure if it also happens in Firefox, I've yet to try it out.

I'm going to investigate this as I continue work on my PRs, and come back with any other info I can spot. Let me know if you can't repro it, it could be local to my machine settings. No other machine available to test on my side atm.

Running it on Win 10 Home (updated to latest version), i5 6200U, integrated GPU.
@VWoeltjen
Copy link
Contributor

Thanks for reporting this. I'm able to reproduce locally; top shows Chrome at 10-25% in Browse, 85-115% in Edit mode for the same layout.

Did some initial investigation. Profiling didn't show any hotspots, and example/profiling showed very modest changes in watch/digest counts. Next I tried using the animation tab in Chrome's dev tools - aha! When animations are paused, the issue goes away.

Based on fiddling with CSS, it seems like it's the pulsing animation pulseNew in particular that is causing this; guessing that is causing more redrawing than we might expect.

Assigning to @charlesh88 to look at performance here. There's an approach for performant animated box shadows that doesn't translate exactly, but might be a decent starting point; basic idea is to use an ::after pseudo-element for the animated part, and animate the opacity on that to reduce the amount of actual redrawing that the browser (thinks it) needs to do.

@VWoeltjen VWoeltjen added this to the Tiptree milestone Jun 1, 2017
@VWoeltjen
Copy link
Contributor

VWoeltjen commented Jun 1, 2017 •
edited

Also, @larkin / @charlesh88, does the "critical" designation seem right to you? This doesn't perceptibly impact performance in my dev environment, but the performance impact is steep enough that I could see this creating problems for users.

@larkin
Copy link
Contributor

larkin commented Jun 2, 2017

@BogdanAlexandru great job reporting this.

Critical, maybe not. We have to be very careful with keyframe animation-- they're not implemented well and generally cause browser repaints at 60hz which causes significant performance overhead. Especially when you consider there are many levels of transparencies and other calculations it's forcing for each frame for every item inside of the edit pane.

I had mentioned this performance problem to @charlesh88 before and he was happy to drop the animation or replace with something different. It's good to get this issue opened and it should be a quick fix.

@larkin
Copy link
Contributor

larkin commented Jun 2, 2017

Here's a good breakdown of the keyframe animation problems from when this was a problem in VSCode: microsoft/vscode#22900

@VWoeltjen
Copy link
Contributor

Removed critical flag per above.

Candidate fix: Render the border by animating the opacity of an ::after pseudo-element with will-change: opacity. Tried locally by editing CSS directly, seems to work.

VWoeltjen added a commit that referenced this issue Jun 2, 2017
@VWoeltjen
[Edit] Move pulse to ::after pseudoelement
b86672d 
Fixes #1603 from a performance perspective; not ready to merge due to
unintended visual effects.
@ghost
Copy link
Author

ghost commented Jun 3, 2017 •
edited by ghost

Thanks a lot for the quick response to this! 👍 It's very appreciated, the issue does hurt when I work in edit mode often, the UI is simply crawling.

@charlesh88
Copy link
Contributor

@BogdanAlexandru Was unaware of this issue about keyframing anims in CSS, thanks for bringing this up. @VWoeltjen, great pointer to a solution. There's other spots where I'm using keyframing, I'm going to look across the app and convert or remove.

charlesh88 added a commit that referenced this issue Jun 9, 2017
@charlesh88
[Frontend] WIP remove keyframe anims
a8a689f 
Fixes #1603
Remove keyframe anims from transition to edit mode
and border-color anim in _messages.scss;
@charlesh88 charlesh88 mentioned this issue Jun 12, 2017
Merged
@charlesh88
Copy link
Contributor

Testing notes:

  • Start Open MCT, make a new Display Layout but don't populate it with anything. Save and exit to Browse mode.
  • In terminal, run top or an equivalent tool that will report on CPU usage of the Chrome browser. Observe the CPU usage for the instance of Chrome that is running Open MCT, which should be somewhere around 15 - 18%. Close other Chrome browser windows if it's unclear which is which.
  • Click to edit the layout. Once in Edit mode, observe the CPU usage of Chrome in top again: it may have gone up while going into Edit mode, but should settle back down again and stay about the same as during Browse mode.

@larkin larkin closed this as completed in c7787aa Jun 21, 2017
larkin pushed a commit that referenced this issue Jun 21, 2017
Squashed commit of the following:
72e9bf3 
commit 73e452e
Author: Pete Richards <peter.l.richards@nasa.gov>
Date:   Wed Jun 21 14:56:51 2017 -0700

    [Imagery] Update spec

commit bbeb97e
Author: Pete Richards <peter.l.richards@nasa.gov>
Date:   Wed Jun 21 14:21:51 2017 -0700

    [Imagery] Use LAD query

commit e6d65f3
Author: Pete Richards <peter.l.richards@nasa.gov>
Date:   Wed Jun 21 14:17:18 2017 -0700

    [Example] Add historic, lad imagery providers

commit 1ab4cf6
Author: Pete Richards <peter.l.richards@nasa.gov>
Date:   Wed Jun 21 13:56:18 2017 -0700

    [example] 5 seconds per image

commit 7fcaf65
Merge: 0713941 44246e6
Author: Pete Richards <pete@pete-richards.com>
Date:   Wed Jun 21 11:24:23 2017 -0700

    Merge pull request #1614 from nasa/open1569

    Fixed tabbing in overlay forms

commit 0713941
Author: Victor Woeltjen <victor.woeltjen@nasa.gov>
Date:   Wed Jun 21 13:23:18 2017 -0500

    [Time] Encode time settings in URL (#1620)

    * [Time Conductor] Skeleton class for URL handling

    #1550

    * [Time Conductor] Scaffold in param handling

    * [Time Conductor] Finish sketching in URL handler

    * [Time Conductor] Usage correct constants for bounds

    * [Time Conductor] Use start/end for bounds/deltas

    * [Time] Move URL handler

    Per discussion, #1550 (comment)

    * [Time] Rename URL handler

    * [Time] Rename URL handler script

    * [Time] Use Time API from URL handler

    * [Time] Utilize Time API

    * [Time] Listen for changes, synchronize URL

    * [Time] Move URL handler into adapter

    ...as it uses Angular constructs.

    * [Time] Wire URL handler into adapter bundle

    * [Time] Pass correct constructor arguments to URL handler

    * [Time] Begin debugging URL handling

    * [Time] Clarify and correct logic for bounds/deltas in URL

    * [Time] Define timeSystem

    * [Time] Pass start/end into time API as numbers

    ...instead of strings.

    * [Time] Avoid creating redundant objects

    * [Time] Restructure fixed vs clock logic

    * [Time] Stop clock correctly for fixed mode

    * [Time] Fix hasBounds/hasDeltas logic

    ...given that both inputs will be strings when read from search params,
    _.isFinite was doomed to return false.

    * [Time] Check for complete information

    ...before updating Time API to reflect search state. Additionally,
    flatten logic to ease readability.

    * [Time] Begin testing TimeSettingsURLHandler

    * [Time] Test fixed mode query string updates

    * [Time] Test realtime mode query string updates

    * [Time] Test update of time API from query params

    * [Time] Add missing semicolons

    Satisfies JSHint; fixes #1550

commit 1d7d56d
Merge: ba99418 61f59a9
Author: Pete Richards <pete@pete-richards.com>
Date:   Wed Jun 21 11:10:14 2017 -0700

    Merge pull request #1613 from nasa/open1592

    [Frontend] Fixed markup to allow scroll of results

commit ba99418
Merge: d3b313d a8a689f
Author: Pete Richards <pete@pete-richards.com>
Date:   Wed Jun 21 11:08:23 2017 -0700

    Merge pull request #1612 from nasa/no-keyframe-anims

    Review and merge no-keyframe-anims branch

commit 44246e6
Author: Charles Hacskaylo <charlesh88@gmail.com>
Date:   Tue Jun 13 14:14:39 2017 -0700

    [Frontend] Modified tabindex of search input

    Fixes #1569

commit 61f59a9
Author: Charles Hacskaylo <charlesh88@gmail.com>
Date:   Tue Jun 13 09:24:42 2017 -0700

    [Frontend] Fixed markup to allow scroll of results

    Fixes #1592

commit a8a689f
Author: Charles Hacskaylo <charlesh88@gmail.com>
Date:   Fri Jun 9 11:40:58 2017 -0700

    [Frontend] WIP remove keyframe anims

    Fixes #1603
    Remove keyframe anims from transition to edit mode
    and border-color anim in _messages.scss;

commit c7787aa
Author: Charles Hacskaylo <charlesh88@gmail.com>
Date:   Tue Jun 6 16:18:22 2017 -0700

    [Frontend] WIP remove keyframe anims

    Fixes #1603
    Remove keyframe anims from transition to edit mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@charlesh88 charlesh88

Labels
None yet
Projects
None yet
Milestone
Tiptree
Development

No branches or pull requests
3 participants
@larkin @charlesh88 @VWoeltjen