Moves Reps from Classes to Functional Components by nchevobbe · Pull Request #122 · firefox-devtools/reps

nchevobbe · 2017-04-06T12:40:56Z

Moving from classes to functions showed a solid 50-60% improvement in my test case (1000 Object reps).

There isn't much drawbacks of doing this since all the component were already stateless.

The only things I had to change were the tests that make sure the root Rep component chose the correct component given a specific grip. The solution I came with is to directly test the getRep function that Rep calls.
I wonder if this is enough though, because of course at some point Rep could do something wrong without being catch by the tests on getRep.
To counter that, what would you think of testing all our components by rendering Rep(props) instead of calling specific functions (e.g. String(props)) ?

Since the components are already stateless, we can transform them into functions without much troubles. This is likely to enhance Reps performance which are a known bottleneck for some tools (e.g. the Console).

The main failure was the one testing that the Rep component was picking up the expected Rep (Grip, TextNode, ...) component given a specific object. The test is modified to use the `getRep` function, that the Rep (from rep.js) calls to render the expected component, since the shallow render would only gives us the type returned by the function, and not the function name.

nchevobbe · 2017-04-10T15:46:57Z

Let's put the review on hold for now, the PR isn't really readable

TODO:

Split functional component changes and safeObjectLink ones
Experiment ES6 classes to see if we get the same performance boost

nchevobbe · 2017-04-11T07:10:24Z

So I set up a quick test with components as ES6 classes on a handful of Reps.
Functional components are still 50% faster than ES6 classes.
I believe it may be because React doesn't do lifecycle checks on functional components, which is likely to improve performance.

For now I'm going to stick with functions, and we'll evaluate introducing stateful components when creating or modifying Reps.

I will now clean up my PR and ask for review when everything is okay.

nchevobbe · 2017-04-11T07:53:53Z

@juliandescottes is so brave he says he can handle it as is. I'll amend the commit message to better reflect what was done (switch to functional component, creation of a safeObjectLink helper, removal of unused methods, better consistency across reps)

juliandescottes

Few nits, but looks great! I tried to be as thorough as possible, I might have missed a few things but I think this is good to go.

Any migration needed in mc in your opinion? (thinking about redundant calls to createFactory?)

juliandescottes · 2017-04-11T08:27:06Z

src/reps/rep-utils.js

  return [];
 }

+function safeObjectLink(props, config, ...children) {


Thanks for moving this to a dedicated util!
Let's add a JSDoc

juliandescottes · 2017-04-11T08:45:28Z

src/reps/rep-utils.js

+}
+
 module.exports = {
  createFactories,


Not to be changed here, but createFactories is no longer used inside of the reps project, only in mc. Maybe we should move it from reps to a shared mc util?

Yes, I agree.
I can still remove it here and file a bug on m-c to move it to a shared util and do the refactor where the one from reps is used for now.
How do you think we should operate ?

I would say :

create a bug in mc to duplicate createFactories from reps in a mc util and use it everywhere

update the image in circle after bug has been resolved

nuke createFactories here

Bug filed https://bugzilla.mozilla.org/show_bug.cgi?id=1355454

juliandescottes · 2017-04-11T09:07:06Z

src/reps/string.js

-});
+function StringRep(props) {
+  let {
+    object: text,


let's add cropLimit since we're using destructuring for this one now.

juliandescottes · 2017-04-11T09:49:03Z

src/reps/array.js

 * and the max number of rendered items depends on the current mode.
 */
-let ArrayRep = React.createClass({
-  displayName: "ArrayRep",


This comment applies to all the migrated components here.
I've been trying to check the behavior of the new functional components when we provide them with invalid/missing props, to check that the propTypes validation was still effective.

For some reason I can't get it to work. Adding a new required propType does not trigger any warning message (I was of course using a dev version of react and making sure that I was calling the rep I modified).

So just as a fair warning, we're probably losing proptypes validation here. I guess it's still good for documentation and that's the main reason we are using it.

Still this "should" work, can't find the reason why it's not doing anything.

I will investigate that.
Maybe what I did in wrapRender isn't enough, I'll check.
It would be nice to still have these warnings

Okay I found it.
I think react does very little with our functional component, and that might include propTypes validation.
If instead of directly calling my rep StringRep(props) I do React.createElement(StringRep, props) I do have the warning messages.
Now I must figure out if doing that will impact the performance of Reps.
If it does, I think we might give Flow a try to replace propTypes maybe ?

Ah great to know where this is coming from at least. Totally agree with your proposal, let's check if there is a performance impact. If there is one, yes flow will be an option.

So I tested it with my test case of 1000 reps (launched 10 times to get an average) :
React.createClass : ~ 3500ms
Functions + React.createElement: ~2200ms
Functions : ~1600ms

Function is definitely faster and I don't think the benefit we get from doing React.createElement (having propTypes warnings) justify the loss of performance.

Thanks for testing! I agree it's not worth the performance impact.

juliandescottes · 2017-04-11T10:54:28Z

src/reps/object.js

+ * Get an array of components representing the properties of the object
+ *
+ * @param {Object} object
+ * @param {Boolean} truncate true if the object will be truncated.


I think it's more accurate to say that truncate is true if the object has been truncated (and maybe rename to truncated ?). We actually truncate when building "interestingObject" and depending on that, we decide to use a comma or not for the last item.

juliandescottes · 2017-04-11T10:56:55Z

src/reps/object.js

-      return propsArray;
-    }
+function propIterator(props, object, max) {
+  let isInterestingProp = (t, value) => {


Maybe use an explicit argument name "type" instead of t (I know this is not really changed by your commit, but since you modified a bit the way we build the props arrays here, I had to read it carefully and the "t"s were pretty annoying :) )

nchevobbe · 2017-04-11T12:48:00Z

Review updated with your feedback @juliandescottes

nchevobbe added 2 commits April 10, 2017 15:51

Modify all Reps to be functions instead of components

1a475a4

Since the components are already stateless, we can transform them into functions without much troubles. This is likely to enhance Reps performance which are a known bottleneck for some tools (e.g. the Console).

nchevobbe force-pushed the perf branch from 950015c to 3c634ff Compare April 10, 2017 14:01

nchevobbe changed the title ~~[WIP] Add a Stress Test Tab~~ Moves Reps from Classes to Functional Components Apr 10, 2017

nchevobbe requested review from janodvarko, jasonLaster and juliandescottes April 10, 2017 14:14

nchevobbe removed request for janodvarko, jasonLaster and juliandescottes April 10, 2017 15:47

nchevobbe changed the title ~~Moves Reps from Classes to Functional Components~~ [WIP] Moves Reps from Classes to Functional Components Apr 10, 2017

nchevobbe requested review from jasonLaster and juliandescottes April 11, 2017 07:51

juliandescottes approved these changes Apr 11, 2017

View reviewed changes

nchevobbe added 2 commits April 11, 2017 13:44

Add JSDoc for safeObjectLink

b489bdd

Better naming

9956b03

nchevobbe changed the title ~~[WIP] Moves Reps from Classes to Functional Components~~ Moves Reps from Classes to Functional Components Apr 11, 2017

Typo

2487c1f

nchevobbe force-pushed the perf branch from 088778f to 2487c1f Compare April 11, 2017 14:48

nchevobbe merged commit 186e174 into master Apr 11, 2017

nchevobbe deleted the perf branch April 11, 2017 15:12

nchevobbe mentioned this pull request Apr 12, 2017

Investigate to improve Reps creation performance #121

Closed

Conversation

nchevobbe commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nchevobbe commented Apr 10, 2017

Uh oh!

nchevobbe commented Apr 11, 2017

Uh oh!

nchevobbe commented Apr 11, 2017

Uh oh!

juliandescottes left a comment

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!

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!

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!

nchevobbe commented Apr 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nchevobbe commented Apr 6, 2017 •

edited

Loading