NetSim: Viz performance cleanup by islemaster · Pull Request #3492 · code-dot-org/code-dot-org

islemaster · 2015-08-11T21:32:42Z

For now, disable the background layer in the visualization. Whether it's causing performance issues or not, it's one of the more obvious symptoms of performance issues when it gets choppy and slow. We're sad to see the floaty network go, but if it helps everything run smoother then it's worth it.
Make sure nodes in the hidden background layer aren't running animation tweens.
For nodes that are running tweens, only update the SVG element's transform attribute when it has actually changed.
Make the background layer easy to re-enable if we want to.

😭 🐼 No more depth-of-field.

Instead, it's up to the client to ask the table for content. For most clients this means calling table.readAll() which returns all cached rows, so the result is the same (this is a little more work, but we can optimize it away as needed). For clients to the log table, the next step is to have them ask for a subset of rows instead of all rows.

It's ugly, but it works. Will do some refactoring.

islemaster · 2015-08-12T05:11:01Z

It's amazing what a difference native DOM manipulation makes. Partial log updates made a HUGE difference while the log is open (and fixed the "scroll back to top" bug) but there was still a long delay anytime we did a full re-render with a lot of rows in the log, like when we open it or when the user changes the sort order.

Here's opening the log before, with < 500 rows in the log. You can clearly see the 5-second churn while it re-renders.

Here's after switching to native DOM manipulation (replacing many $('<td>') calls with document.createElement('td'), etc). The full refresh has condensed to about 100ms.

islemaster · 2015-08-12T16:20:05Z

For documentation: Yes, this PR has expanded beyond visualization optimizations. In particular, it contains a series of improvements to the log browser which we identified as a major offender in the performance space. Here's the history of updates to that feature:

Originally, it responded to every log-table-change event with a full re-render, even when hidden. In PR 3362 @Hamms detaches the log table change monitoring when the log browser is hidden, significantly reducing extra rendering overhead.

Even so, our polling fallback was causing a re-render every 10 seconds. 02206b8 stops the log browser from rendering at all if it is not visible (and forces an initial render when it is opened)

This meant a massive improvement to performance when the log browser is closed, but it still became nigh-unusable when open and displaying several hundred rows, because on every change notification it would re-decode every log packet, and re-render the entire log table. Re-rendering the full table also caused a bug where the scroll position would be reset with every update.

b111ba1 is a preparation step. It changes the signature of the table change callbacks so that they no longer receive the full table contents as an argument, and must explicitly request needed rows from the local cache. This model lines up better with the refresh/readAll split anyway, which happened back in PR 3361.

d06d181 causes the log browser to only request and decode new rows on update, not all of them, reducing redundant base64-decoding. The browser still re-renders the full UI every time.

cfe426a (and the clean-up in c52f5af) introduce partial DOM updates on change events, simply inserting the new rows at the appropriate positions instead of re-rendering everything. This provides a massive performance improvement when the log browser is open. However, it still does full re-renders when opened and when changing sort or filter preferences, which can still be terribly slow (5 seconds or more!).

ebbbbb2 optimizes DOM creation in the log browser, providing at least a tenfold improvement in the full-refresh time.

islemaster · 2015-08-12T17:06:49Z

@philbogle Please review these optimization when you can today. Elijah is working on one more change that might go into this (removing extra wire rendering) but it should be easy to review independently.

Only makes DOM updates for the VizWires when we can detect that something's changed

philbogle · 2015-08-12T19:51:07Z

iteratee is an unconventional name, I first thought it was a typo. Can you come up with something more descriptive of what is being iterated?

Renaming:
iterateeMap => sortKeyToSortValueGetterMap
iterateeFunction => getSortValue

iterateeMap => sortKeyToSortValueGetterMap iterateeFunction => getSortValue

islemaster · 2015-08-12T21:04:52Z

@philbogle responded to your first few comments. Anything else?

philbogle · 2015-08-12T21:20:48Z

Couldn't it be the case that there are leftover rows from nextOld that need to be merged?

Would be nice if there is a way to test this! Given the dependency on the DOM, is there any way to do that that wouldn't be too difficult? I'm interested in cases like:

added rows at end of the table

no rows added

initially empty table

etc.

Oh yeah. Actually, I can do a bit of testing around what it does to the DOM.

Regarding your first question, oldRows are already in the DOM, so they don't need to be added.

Added some tests over on #3513 - it could always use some more though.

philbogle · 2015-08-12T21:42:01Z

Please add trailing newline if possible.

Done on #3513

…anup NetSim: Viz performance cleanup

commit 836a717 Merge: 1788d65 0cde9b9 Author: Bjvanminnen <Bjvanminnen@gmail.com> Date: Wed Aug 12 15:06:04 2015 -0700 Merge pull request #3252 from code-dot-org/hackedLevels custom type for play lab ninja cat commit 1788d65 Merge: 65b860e 7f669b3 Author: Brian Jordan <bcjordan@gmail.com> Date: Wed Aug 12 15:04:13 2015 -0700 Merge pull request #3512 from code-dot-org/fix_example_buttons_ie11 [Finishes #101164984] Fix pressing example add/run buttons on IE11. commit 0cde9b9 Author: Brent Van Minnen <bjvanminnen@gmail.com> Date: Tue Jul 28 10:07:44 2015 -0700 custom type for play lab ninja cat commit 7f669b3 Author: Brian Jordan <bcjordan@gmail.com> Date: Wed Aug 12 15:03:42 2015 -0700 [Finishes #101164984] Fix example add/run buttons on IE11. commit 65b860e Merge: fef04cd 99c5650 Author: Bjvanminnen <Bjvanminnen@gmail.com> Date: Wed Aug 12 15:00:49 2015 -0700 Merge pull request #3501 from code-dot-org/examplesOnClose [Finishes #99188090] Examples on close commit fef04cd Merge: 5fce434 550c909 Author: philbogle <phil@code.org> Date: Wed Aug 12 14:59:45 2015 -0700 Merge pull request #3511 from code-dot-org/revert-3492-netsim-viz-performance-cleanup Revert "NetSim: Viz performance cleanup" commit 550c909 Author: philbogle <phil@code.org> Date: Wed Aug 12 14:59:01 2015 -0700 Revert "NetSim: Viz performance cleanup" commit 5fce434 Merge: 6677a1d e6d07ba Author: philbogle <phil@code.org> Date: Wed Aug 12 14:57:14 2015 -0700 Merge pull request #3492 from code-dot-org/netsim-viz-performance-cleanup NetSim: Viz performance cleanup commit 6677a1d Author: Continuous Integration <dev@code.org> Date: Wed Aug 12 21:47:41 2015 +0000 Automatically built. commit a4aefe4 Merge: 3848999 89a2f53 Author: Brian Jordan <bcjordan@gmail.com> Date: Wed Aug 12 14:33:20 2015 -0700 Merge pull request #3508 from code-dot-org/ignore_clicks_drawn_ui Ignore clicks on SVG-drawn UI elements to allow panning through them. commit 89a2f53 Author: Brian Jordan <bcjordan@gmail.com> Date: Wed Aug 12 14:31:06 2015 -0700 Rename svgIgnoreMouse [ci skip] commit 96647d1 Author: Brian Jordan <bcjordan@gmail.com> Date: Wed Aug 12 14:20:51 2015 -0700 Ignore clicks on SVG-drawn UI elements to allow panning through them. - Rename callText to callResult text to be more accurate (result also uses it) - Add helper for adding pointer events for SVG elements commit 99c5650 Author: Brent Van Minnen <bjvanminnen@gmail.com> Date: Wed Aug 12 14:37:17 2015 -0700 finish bad rebase commit 3bf61c6 Merge: a4aefe4 0300263 Author: Tanya Parker <tanya@code.org> Date: Wed Aug 12 14:36:46 2015 -0700 Merge pull request #3509 from code-dot-org/strayunderscore no text decoration on buttons commit 9d6c0bc Author: Brent Van Minnen <bjvanminnen@gmail.com> Date: Wed Aug 12 14:34:21 2015 -0700 fix UI test commit 56c2f28 Author: Brent Van Minnen <bjvanminnen@gmail.com> Date: Wed Aug 12 13:33:17 2015 -0700 code review feedback commit bc16af5 Author: Brent Van Minnen <bjvanminnen@gmail.com> Date: Wed Aug 12 12:49:14 2015 -0700 consolidate some common code

Hamms and others added 5 commits August 10, 2015 18:00

completing TODO; only update the DOM when it's out of date

92a6c22

Extract NetSimVisualization markup from page.html.ejs

2a7af25

Simply remove the blur and hide the background layer.

04403ec

Only search for groups once, fix some jsdoc

fb5a60b

Disable animations for nodes in background layer

83a6071

islemaster assigned Hamms Aug 11, 2015

islemaster added 7 commits August 11, 2015 16:00

Remove outdated TODO

8582a2f

Don't render log modal if it's not visible.

02206b8

Router log modal requests and decodes deltas, not full table

d06d181

Partial log browser DOM updates.

cfe426a

It's ugly, but it works. Will do some refactoring.

Refactor and clean up partial log browser update code

c52f5af

Faster, non-jquery log row creation

ebbbbb2

islemaster added 2 commits August 12, 2015 09:35

Limit full log browser render to 500 rows.

080b120

Add message at end of log if rendered messages were limited.

db113bf

islemaster assigned philbogle and unassigned Hamms Aug 12, 2015

More clever wire rendering

4e6b260

Only makes DOM updates for the VizWires when we can detect that something's changed

philbogle reviewed Aug 12, 2015
View reviewed changes

islemaster added 3 commits August 12, 2015 13:32

JSHint fix

452ef3f

Rename 'iteratee*'

8eb56fb

iterateeMap => sortKeyToSortValueGetterMap iterateeFunction => getSortValue

More readable merging logic in log browser

e6d07ba

philbogle reviewed Aug 12, 2015
View reviewed changes

philbogle added a commit that referenced this pull request Aug 12, 2015

Merge pull request #3492 from code-dot-org/netsim-viz-performance-cle…

5fce434

…anup NetSim: Viz performance cleanup

philbogle merged commit 5fce434 into staging Aug 12, 2015

philbogle mentioned this pull request Aug 12, 2015

Revert "NetSim: Viz performance cleanup" #3511

Merged

islemaster mentioned this pull request Aug 12, 2015

Unrevert "NetSim: Viz performance cleanup" #3513

Merged

islemaster deleted the netsim-viz-performance-cleanup branch November 3, 2015 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

NetSim: Viz performance cleanup#3492

NetSim: Viz performance cleanup#3492
philbogle merged 18 commits into
stagingfrom
netsim-viz-performance-cleanup

islemaster commented Aug 11, 2015

Uh oh!

islemaster commented Aug 12, 2015

Uh oh!

islemaster commented Aug 12, 2015

Uh oh!

islemaster commented Aug 12, 2015

Uh oh!

philbogle Aug 12, 2015

Uh oh!

islemaster Aug 12, 2015

Uh oh!

islemaster commented Aug 12, 2015

Uh oh!

philbogle Aug 12, 2015

Uh oh!

philbogle Aug 12, 2015

Uh oh!

islemaster Aug 12, 2015

Uh oh!

islemaster Aug 12, 2015

Uh oh!

philbogle Aug 12, 2015

Uh oh!

islemaster Aug 12, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

islemaster commented Aug 11, 2015

Uh oh!

islemaster commented Aug 12, 2015

Uh oh!

islemaster commented Aug 12, 2015

Uh oh!

islemaster commented Aug 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

islemaster commented Aug 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants