Unperformant Stringify Warning - WIP #12263

Swieckowski · 2018-02-21T20:02:08Z

Closes #12209.
performance.now() was not supported while I was writing this, so I'm using the less precise Date.now().

I have written a test to see if the warning shows up if something takes too long to stringify, but don't have one for if it does not warn, since I didn't see anyone else test for anything like that.

…e values and warns if they take too long

xilaraux · 2018-02-21T21:23:51Z

@Swieckowski
What did you mean by

performance.now() was not supported

Swieckowski · 2018-02-21T21:30:42Z

In the simplest terms, if I run yarn test I'll fail a bunch of tests with ReferenceError: performance is not defined.

xilaraux · 2018-02-21T22:32:45Z

I see, thx for explanation.

pshrmn · 2018-02-21T23:55:45Z

Node 8.5 is required for performance.now, but devEngines specifies that all 8.X versions are supported. I don't know what the process is for deciding which Node versions to support, so I'm not sure if 8.5 can just be made the minimum version (it looks like 8.9 is LTS).

viankakrisna · 2018-02-22T04:37:12Z

packages/react-dom/src/client/DOMPropertyOperations.js

      } else {
-        node.setAttribute(attributeName, '' + (value: any));
+        node.setAttribute(attributeName, (value: any).toString());


isn't this will throw on undefined value? we only strictly check for null above it.

Yeah, @Swieckowski please revert this. We were intentionally not using toString

n3tr · 2018-03-01T07:03:56Z

Can we do check if performance.now is available and fallback to Date.now() if it's not, like in ReactNativeFrameScheduling.js

react/packages/react-native-renderer/src/ReactNativeFrameScheduling.js

Lines 12 to 17 in 2cf9063

    
           const hasNativePerformanceNow = 
        
             typeof performance === 'object' && typeof performance.now === 'function'; 
        
           const now = hasNativePerformanceNow 
        
             ? () => performance.now() 
        
             : () => Date.now();

n3tr · 2018-03-01T07:22:49Z

packages/react-dom/src/__tests__/DOMPropertyOperations-test.js

+    it('should warn if custom attributes take too long to stringify', () => {
+      const container = document.createElement('div');
+      const attributeValue = {foo: 'bar'};
+      attributeValue.toString = function() {


IMO, We don't need actual implementation (which will slow down the test suite). I think better do polyfill global.performance in the test setup and implement our own version of it (like @aweary mentioned in #12209 (comment)).

For an idea, we can take a look at

react/packages/react-dom/src/__tests__/ReactDOMRoot-test.internal.js

Lines 49 to 53 in 2cf9063

global.performance = {

now() {

return now;

},

};

or jest.mockImplementation might be helpful.

Hope this help :)

Yeah, let's go ahead and go with mocking performance instead of actually taking too much time.

raunofreiberg · 2018-03-05T07:35:55Z

packages/react-dom/src/client/DOMPropertyOperations.js

+// Only used in DEV, stringifies a value and Warns if it took too long
+const stringifyWithPerformanceWarning = value => {
+  const stringifyStart = Date.now();
+  let attributeValue = (value: any).toString();


Use const over let.

Please use '' + value instead of toString here as well.

raunofreiberg · 2018-03-05T07:38:11Z

packages/react-dom/src/client/DOMPropertyOperations.js

+
+  warning(
+    stringifyEnd - stringifyStart <= 2,
+    'Stringifying your attribute is causing perfomance issues',


Typo: pefomance => performance.

The messaging here isn't clear enough. Let's also pass the attribute name to stringifyWithPerformanceWarning, improve the wording, and also include a component stack. You can get a component stack by importing and using getCurrentFiberStackAddendum from ReactDebugCurrentFiber.

Lets make it look like:

warning( stringifyEnd - stringifyStart <= 2, 'The attribute `%s` took more than 2 milliseconds to stringify. This usually means you provided a large object ' + 'as the value for a DOM attribute, which can lead to performance issues.%s', attributeName, getCurrentFiberStackAddendum(), )

raunofreiberg · 2018-03-05T07:52:29Z

packages/react-dom/src/client/DOMPropertyOperations.js

    } else {
      // `setAttribute` with objects becomes only `[object]` in IE8/9,
      // ('' + value) makes it output the correct toString()-value.
-      attributeValue = '' + (value: any);
+      attributeValue = (value: any).toString();


Why was this changed? The comment above no longer reflects the actual code. Aren't we concerned about IE8/9 anymore? If not, then the comment should also be updated, I guess.

https://reactjs.org/blog/2016/01/12/discontinuing-ie8-support.html

Just IE9+ it seems

@Swieckowski please revert his back as well.

aweary

Sorry for the late review, and thanks again for doing this @Swieckowski. Please see my comments for what we need to change.

aweary · 2018-03-27T23:10:44Z

packages/react-dom/src/__tests__/DOMPropertyOperations-test.js

+    it('should warn if custom attributes take too long to stringify', () => {
+      const container = document.createElement('div');
+      const attributeValue = {foo: 'bar'};
+      attributeValue.toString = function() {


Yeah, let's go ahead and go with mocking performance instead of actually taking too much time.

aweary · 2018-03-27T23:11:41Z

packages/react-dom/src/client/DOMPropertyOperations.js

      } else {
-        node.setAttribute(attributeName, '' + (value: any));
+        node.setAttribute(attributeName, (value: any).toString());


Yeah, @Swieckowski please revert this. We were intentionally not using toString

aweary · 2018-03-27T23:12:22Z

packages/react-dom/src/client/DOMPropertyOperations.js

    } else {
      // `setAttribute` with objects becomes only `[object]` in IE8/9,
      // ('' + value) makes it output the correct toString()-value.
-      attributeValue = '' + (value: any);
+      attributeValue = (value: any).toString();


@Swieckowski please revert his back as well.

aweary · 2018-03-27T23:14:40Z

packages/react-dom/src/client/DOMPropertyOperations.js

@@ -172,3 +180,17 @@ export function setValueForProperty(
    }
  }
 }
+
+// Only used in DEV, stringifies a value and Warns if it took too long
+const stringifyWithPerformanceWarning = value => {


For consistency let's move this to the top of the file, and then only define it in DEV

let stringifyWithPerformanceWarning; if (__DEV__) { stringifyWithPerformanceWarning = function(value) { // ... } } ``

aweary · 2018-03-27T23:15:25Z

packages/react-dom/src/client/DOMPropertyOperations.js

+// Only used in DEV, stringifies a value and Warns if it took too long
+const stringifyWithPerformanceWarning = value => {
+  const stringifyStart = Date.now();
+  let attributeValue = (value: any).toString();


Please use '' + value instead of toString here as well.

aweary · 2018-03-27T23:17:43Z

packages/react-dom/src/client/DOMPropertyOperations.js

+
+// Only used in DEV, stringifies a value and Warns if it took too long
+const stringifyWithPerformanceWarning = value => {
+  const stringifyStart = Date.now();


We want to use performance.now if it exists, and fallback to Date.now if it doesn't. Lets do the same thing we do here: https://github.com/facebook/react/blob/master/packages/react-native-renderer/src/ReactNativeFrameScheduling.js#L12-L17

Thanks a lot for the review, I'll get on it sometime soon!

aweary · 2018-03-27T23:24:03Z

packages/react-dom/src/client/DOMPropertyOperations.js

+
+  warning(
+    stringifyEnd - stringifyStart <= 2,
+    'Stringifying your attribute is causing perfomance issues',


The messaging here isn't clear enough. Let's also pass the attribute name to stringifyWithPerformanceWarning, improve the wording, and also include a component stack. You can get a component stack by importing and using getCurrentFiberStackAddendum from ReactDebugCurrentFiber.

Lets make it look like:

warning( stringifyEnd - stringifyStart <= 2, 'The attribute `%s` took more than 2 milliseconds to stringify. This usually means you provided a large object ' + 'as the value for a DOM attribute, which can lead to performance issues.%s', attributeName, getCurrentFiberStackAddendum(), )

gaearon · 2018-08-09T00:48:00Z

One more concern is that performance.now() itself might add overhead. We'd want to measure the impact of this on a large tree.

hassaans · 2019-07-17T02:31:04Z

We'll only be showing the warning in development mode. Performance drops in development should be insignificant.

stale · 2020-01-10T05:52:36Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

stale · 2020-01-17T05:58:55Z

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

Swieckowski added 8 commits February 14, 2018 17:37

created function for use in DEV environment that stringifies attribut…

b294360

…e values and warns if they take too long

formatting

969268e

created warning tests

d4c1c5f

testing test

ce53b1d

finished up date.now build and tests

9500ee4

removed unnecessary test

2f4dad4

ran prettier

bce68be

updating forked repo

23ea3d2

facebook-github-bot added the CLA Signed label Feb 21, 2018

viankakrisna reviewed Feb 22, 2018

View reviewed changes

n3tr reviewed Mar 1, 2018

View reviewed changes

raunofreiberg reviewed Mar 5, 2018

View reviewed changes

aweary suggested changes Mar 27, 2018

View reviewed changes

klamping mentioned this pull request Apr 22, 2018

React PR Test Report Listing klamping/react-webdriverio-tests#1

Open

stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020

stale bot closed this Jan 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unperformant Stringify Warning - WIP #12263

Unperformant Stringify Warning - WIP #12263

Swieckowski commented Feb 21, 2018

xilaraux commented Feb 21, 2018

Swieckowski commented Feb 21, 2018

xilaraux commented Feb 21, 2018

pshrmn commented Feb 21, 2018

viankakrisna Feb 22, 2018

aweary Mar 27, 2018

n3tr commented Mar 1, 2018

n3tr Mar 1, 2018

aweary Mar 27, 2018

raunofreiberg Mar 5, 2018

aweary Mar 27, 2018

raunofreiberg Mar 5, 2018

aweary Mar 27, 2018

raunofreiberg Mar 5, 2018

quantizor Mar 8, 2018

aweary Mar 27, 2018

aweary left a comment

aweary Mar 27, 2018

aweary Mar 27, 2018

aweary Mar 27, 2018

aweary Mar 27, 2018

aweary Mar 27, 2018

aweary Mar 27, 2018

Swieckowski Mar 27, 2018

aweary Mar 27, 2018

gaearon commented Aug 9, 2018

hassaans commented Jul 17, 2019

stale bot commented Jan 10, 2020

stale bot commented Jan 17, 2020

Unperformant Stringify Warning - WIP #12263

Unperformant Stringify Warning - WIP #12263

Conversation

Swieckowski commented Feb 21, 2018

xilaraux commented Feb 21, 2018

Swieckowski commented Feb 21, 2018

xilaraux commented Feb 21, 2018

pshrmn commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n3tr commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aweary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Aug 9, 2018

hassaans commented Jul 17, 2019

stale bot commented Jan 10, 2020

stale bot commented Jan 17, 2020