1313161 - Improve performance of Reps

Status: RESOLVED WONTFIX

(DevTools :: Shared Components, defect)

Description

[Lin Clark [:linclark]]

Formatting grip values using reps is 2-4x slower than outputting the values without Reps. Because rendering reps is so heavy, if the console uses React Virtualized (Bug 1308216) it will flicker and the scroll will jank as new reps come into view.

See Bug 1312737 for testing, and Bug 1312776 for additional experiments.

Comment 1

[Lin Clark [:linclark]]

Attachment: Reduce number of components

This patch is a proof of concept. It demonstrates removing the use of React components where they are not necessary.

In my tests on my machine:

Logging 1000 numbers - 700ms before, 415ms after (IIRC)
Logging 100 nested objects - 615ms before, 440ms after

This likely still doesn't bring us down to the level of performance we need for console, but it's a start. Honza, what would you think of this? FYI, I will likely not have time to complete this before I switch teams, so if you have someone you can put on it, feel free.

Comment 2

[Lin Clark [:linclark]]

Here's a Talos job that shows the difference with objects. Not high confidence yet (I just retriggered the jobs), but it is currently showing a 15% improvement.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=9c1b74942675&newProject=try&newRevision=d5e3a8cf1020&framework=1&showOnlyImportant=0

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Lin Clark [:linclark] from comment #1)
> Created attachment 8804874 [details] [diff] [review]
> Reduce number of components
> 
> This patch is a proof of concept. It demonstrates removing the use of React
> components where they are not necessary.
> 
> In my tests on my machine:
> 
> Logging 1000 numbers - 700ms before, 415ms after (IIRC)
> Logging 100 nested objects - 615ms before, 440ms after
> 
> This likely still doesn't bring us down to the level of performance we need
> for console, but it's a start. Honza, what would you think of this? FYI,
I definitely support using stateless functional components so, I think this is step in the right direction.

> I will likely not have time to complete this before I switch teams, so if you
> have someone you can put on it, feel free.
I see, thanks for heads up.


Honza

Comment 4

[Lin Clark [:linclark]]

It's worth noting, this POC doesn't use stateless functional components. I tested those and IIRC they still had about the same performance cost.

Instead, what I did in this POC is just use functions. Those functions prepare and return the ReactDOM components (like span), but aren't components themselves.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Lin Clark [:linclark] from comment #4)
> It's worth noting, this POC doesn't use stateless functional components. I
> tested those and IIRC they still had about the same performance cost.
Ah, I see

> Instead, what I did in this POC is just use functions. Those functions
> prepare and return the ReactDOM components (like span), but aren't
> components themselves.
Understand


A few comments:

1) It make good sense to me to avoid too many components and save mounting cost. But, I am not entirely sure if we are not loosing too much by converting all components into simple functions. We might want to implement more complex components in the future that offer better user interaction e.g. expanding objects for inline inspection, preview for existing data structures on the page [1], HTTP inspector is also quite interactive, etc. 

2) We might want to use different rendering approach for Reps (not using React). See e.g. Jason's experiment here: https://github.com/jasonLaster/vanilla-dom. We could use similar syntax, but simpler and faster implementation (to replace React). Again, dropping React for all Reps might be too much but, some nice integration of both could work.

3) The previous example is based on createElement API, but I recall that using DOM API too much wasn't fast either. Perhaps we could come up with a solution that generates HTML markup as a string and sets it to `innerHTML`.

4) We can also limit number of things rendered in the Console. E.g. instead of rendering 300 items max in an array, we can do only 50 (+ more link), etc. AFAIU the perf issue is related mainly to arrays with a lot of items and objects with a lot of props. Both can be limited and so number of rendered components. Perhaps existing Reps could be rather optimized for corner cases instead of converted into functions.


I originally wanted to help with this but, I can't find time. I can do reviews of suggested solutions though!


@Brian, I am marking this as blocker for bug 1308219 but, feel free to change if needed.


Honza


[1] E.g. preview for `performance.timing` data, http://www.softwareishard.com/blog/firebug/support-for-performance-timing-in-firebug/

Comment 6

[Lin Clark [:linclark]]

Just chiming in here since I spent the last few weeks deep in the perf work.

> 2) We might want to use different rendering approach for Reps (not using React). See e.g. Jason's
> experiment here: https://github.com/jasonLaster/vanilla-dom. We could use similar syntax, but simpler
> and faster implementation (to replace React). Again, dropping React for all Reps might be too much but,
> some nice integration of both could work.

If I'm thinking through it properly, you'd want to watch out for memory leaks with an approach like this. This seems to go pretty against the programming paradigm of React, so it could end up causing more problems in the end, both in ensuring it doesn't do anything strange now, and also in the future as React changes with things like Fiber. And wouldn't this mean that you lose the things mentioned in 1 anyway? How would the updates happen?

> 3) The previous example is based on createElement API, but I recall that using DOM API too much wasn't
> fast either. Perhaps we could come up with a solution that generates HTML markup as a string and sets it
> to `innerHTML`.

In modern browsers, this is actually slower than the DOM API. Ben Alpert on the React team did some tests, and it motivated their switch from innerHTML in .14 to DOM in 15: https://github.com/spicyj/innerhtml-vs-createelement-vs-clonenode

> We can also limit number of things rendered in the Console. E.g. instead of rendering 300 items max in
> an array, we can do only 50 (+ more link), etc. AFAIU the perf issue is related mainly to arrays with a 
> lot of items and objects with a lot of props. Both can be limited and so number of rendered components.
> Perhaps existing Reps could be rather optimized for corner cases instead of converted into functions.

"The perf issue is related to arrays with a lot of items" assumes the the user is logging single objects, rather than lots of objects. I'm not sure if that's an accurate assumption or not. 

What if someone is logging a bunch of simple objects, but all at the same time? You can use console.log(...Array(100).fill(i).join(" ")) in a for loop to see the problem. There's not good data on how users use the console. This may be something people do, or might not be.

If features are prioritized over perf here, then pathological cases will be easy to run into (by logging multiple objects in one row). I'm not saying that's a tradeoff that shouldn't be made, but it's a tradeoff that should be made consciously.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Lin Clark [:linclark] from comment #6)
> If I'm thinking through it properly, you'd want to watch out for memory
> leaks with an approach like this. This seems to go pretty against the
> programming paradigm of React, so it could end up causing more problems in
> the end, both in ensuring it doesn't do anything strange now, and also in
> the future as React changes with things like Fiber. And wouldn't this mean
> that you lose the things mentioned in 1 anyway? How would the updates happen?
All good points. I don't have a precise thought-out plan here.
The premise in this bullet point is just "let's use something that's faster than React".

> In modern browsers, this is actually slower than the DOM API. Ben Alpert on
> the React team did some tests, and it motivated their switch from innerHTML
> in .14 to DOM in 15:
> https://github.com/spicyj/innerhtml-vs-createelement-vs-clonenode
Ah nice, so yes using innerHTML sounds irrelevant.


> "The perf issue is related to arrays with a lot of items" assumes the the
> user is logging single objects, rather than lots of objects. I'm not sure if
> that's an accurate assumption or not. 
> 
> What if someone is logging a bunch of simple objects, but all at the same
> time? You can use console.log(...Array(100).fill(i).join(" ")) in a for loop
> to see the problem. There's not good data on how users use the console. This
> may be something people do, or might not be.
Could we also limit number of arguments passed in console API?


Honza

> If features are prioritized over perf here, then pathological cases will be
> easy to run into (by logging multiple objects in one row). I'm not saying
> that's a tradeoff that shouldn't be made, but it's a tradeoff that should be
> made consciously.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Jan Honza Odvarko [:Honza] from comment #7)
> > "The perf issue is related to arrays with a lot of items" assumes the the
> > user is logging single objects, rather than lots of objects. I'm not sure if
> > that's an accurate assumption or not. 
> > 
> > What if someone is logging a bunch of simple objects, but all at the same
> > time? You can use console.log(...Array(100).fill(i).join(" ")) in a for loop
> > to see the problem. There's not good data on how users use the console. This
> > may be something people do, or might not be.
> Could we also limit number of arguments passed in console API?

No, I'd rather be right than fast for logged values.  I suspect also that this is not common (although don't have data to back it up other than observed usage of the console)

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> (In reply to Jan Honza Odvarko [:Honza] from comment #7)
> > > "The perf issue is related to arrays with a lot of items" assumes the the
> > > user is logging single objects, rather than lots of objects. I'm not sure if
> > > that's an accurate assumption or not. 
> > > 
> > > What if someone is logging a bunch of simple objects, but all at the same
> > > time? You can use console.log(...Array(100).fill(i).join(" ")) in a for loop
> > > to see the problem. There's not good data on how users use the console. This
> > > may be something people do, or might not be.
> > Could we also limit number of arguments passed in console API?
> 
> No, I'd rather be right than fast for logged values.  I suspect also that
> this is not common (although don't have data to back it up other than
> observed usage of the console)

To be clear, I'm not saying we can't make simplifications or changes in how reps are displayed in certain cases - just that removing values that were in fact logged is a non starter.

To frame the performance discussion, I would pick an arbitrary number and say we should be able to render 5 reps in a single message easily (and not worry about 100+ reps in a single message).

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

There don't seem to be anything actionable for that bug at the moment.
We already did what we could do on the Reps side (moving to pure functions) and I don't think there isn't any more room for perf improvement there (and if there is, we would do that work in the devtools-core repo on Github)