Skip to content

Commit

Permalink
Show React events in the timeline when ReactPerf is active (#7549)
Browse files Browse the repository at this point in the history 
* Show React events in the timeline when ReactPerf is active

This currently only seems to work on Chrome.

* Address Chrome issue

(cherry picked from commit 0a248ee)
  • Loading branch information
@gaearon @zpao
gaearon authored and zpao committed Oct 3, 2016
1 parent ecdc185 commit cbc4508
Showing 1 changed file with 65 additions and 0 deletions.
65 changes: 65 additions & 0 deletions src/renderers/shared/ReactDebugTool.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,63 @@ function resumeCurrentLifeCycleTimer() {
currentTimerType = timerType;
}

var lastMarkTimeStamp = null;
var canUsePerformanceMeasure =
typeof performance !== 'undefined' &&
typeof performance.mark === 'function' &&
typeof performance.clearMarks === 'function' &&
typeof performance.measure === 'function' &&
typeof performance.clearMeasures === 'function';

function shouldMark(debugID) {
if (!isProfiling || !canUsePerformanceMeasure) {
return false;
}
var element = ReactComponentTreeHook.getElement(debugID);
if (element == null || typeof element !== 'object') {
return false;
}
var isHostElement = typeof element.type === 'string';
if (isHostElement) {
return false;
}
return true;
}

function markBegin(debugID, markType) {
if (!shouldMark(debugID)) {
return;
}

var markName = `${debugID}::${markType}`;
lastMarkTimeStamp = performanceNow();
performance.mark(markName);
}

function markEnd(debugID, markType) {
if (!shouldMark(debugID)) {
return;
}

var markName = `${debugID}::${markType}`;
var displayName = ReactComponentTreeHook.getDisplayName(debugID);

// Chrome has an issue of dropping markers recorded too fast:
// https://bugs.chromium.org/p/chromium/issues/detail?id=640652
// To work around this, we will not report very small measurements.
// I determined the magic number by tweaking it back and forth.
// 0.05ms was enough to prevent the issue, but I set it to 0.1ms to be safe.
// When the bug is fixed, we can `measure()` unconditionally if we want to.
var timeStamp = performanceNow();
if (timeStamp - lastMarkTimeStamp > 0.1) {
var measurementName = `${displayName} [${markType}]`;
performance.measure(measurementName, markName);

This comment has been minimized.

Sign in to view

Copy link
@davidrenne

davidrenne May 15, 2017

Contributor

Hey @gaearon it's nice to interact on code from the hot reload and redux master himself!

I am having issues with this performance.measure where it cannot find the starting mark in my application. I am on version 15.4.2 of react and nothing has changed with this in current master it is still not handling errors if an application is setup in a way that confuses this logger's "start measurement" where the key of the measurementName does not exist upon calling markEnd.

Basically, this breaks my entire app certain modules because it cannot render. Wouldn't a try/catch be a nice thing here if somehow an error occurs, the app will just log that an error occurred in performance benchmarking the renders and the developer just has his app rendering just fine, but just has an error.

Rather the way it is coded, its a fatal error and uncaught exception so the entire render fails to even load because this is not handled with a try/catch.

Any thoughts on adding this? For now I can just turn off performance rendering for developers or fork this version of react to put a try/catch in myself until this is handled better. In my opinion, a performance monitoring solution should never affect an apps render since all of these are just logged and the core library should never have uncaught exceptions raising up the stack and causing render to fail.

This comment has been minimized.

Sign in to view

Copy link
@gaearon

gaearon May 15, 2017 •
edited
Loading

Author Collaborator

We already do this in version 16, but in your case it likely means you have an earlier error your app is swallowing. This causes mark/measure calls to misalign. This is common if you incorrectly use Promises and forget to log caught errors. Please use "pause on caught exceptions" and you'll find it!

This comment has been minimized.

Sign in to view

Copy link
@davidrenne

davidrenne May 15, 2017

Contributor

Hey Dan, i tried pausing on exceptions and I did find a websocket callback is causing some issues on our end where it did not have a try/catch. But still I think this line of code in any 15.x versions is the ultimate culprit in things not rendering because even after I cleared up what I thought was causing this, there still is an issue that the performance never was started in the first place for these particular components and it cannot measure the performance of something that never started.

This comment has been minimized.

Sign in to view

Copy link
@gaearon

gaearon May 15, 2017

Author Collaborator

If it was never started then it still sounds like there's a bug in your code. I agree it should be more resilient (and it will be) but what you describe sounds like an application bug. I'd appreciate if you could trim it down to a small reproducing case, and I can take a look then.

This comment has been minimized.

Sign in to view

Copy link
@davidrenne

davidrenne May 15, 2017

Contributor

Since 16 isnt out, is there a way to retrofit the try catch on your end to a future version in the 15.x tags of react? Maybe with a minor upgrade including some of 16.0's code. Yea i dont know how to quite get a small recreatable version, but if I find some time maybe I will try one so we can see why exactly the original caller doesnt have a window.performance entry in the first place.

This comment has been minimized.

Sign in to view

Copy link
@gaearon

gaearon May 15, 2017

Author Collaborator

Since 16 isnt out, is there a way to retrofit the try catch on your end to a future version in the 15.x tags of react?

No, 16 is a complete rewrite.

Yea i dont know how to quite get a small recreatable version, but if I find some time maybe I will try one so we can see why exactly the original caller doesnt have a window.performance entry in the first place.

Please do. I'm also happy to take a PR that wraps measure() call in a try-catch for the next 15.x release if you'd like. If you decide to submit it, please do against 15-stable branch.

This comment has been minimized.

Sign in to view

Copy link
@davidrenne

davidrenne May 15, 2017

Contributor

@gaearon hey are we sure the try/catch is in master/v16?

https://github.com/facebook/react/blob/master/src/renderers/shared/ReactDebugTool.js#L302

Maybe its in another branch I dont know about which v16 is being developed? I was going to copy/paste some code for a PR.

This comment has been minimized.

Sign in to view

Copy link
@gaearon

gaearon May 15, 2017

Author Collaborator

That file will be removed in final 16. It's not used by the main code path anymore, but left for a few things we haven't ported to new engine yet.

This comment has been minimized.

Sign in to view

Copy link
@davidrenne

davidrenne May 15, 2017

Contributor

Ahh thank you. So you are just fine with something like this then right?

https://github.com/davidrenne/react/blob/15-stable/src/renderers/shared/ReactDebugTool.js#L290-L294

I wasnt sure exactly what are the standards you guys like doing on catching errors so please advise if something like that is the direction that you have in the v16 try/catch. Wherever that code is living currently LOL! This way they can be consistent across versions.

This comment has been minimized.

Sign in to view

Copy link
@davidrenne

davidrenne May 15, 2017

Contributor

LOL, and I am hard coding english strings. That could be bad for other people in the world. Just give me some advice on how the format should be like.

This comment has been minimized.

Sign in to view

Copy link
@gaearon

gaearon May 15, 2017

Author Collaborator

In this case it's fine to just swallow the error silently and not print anything since it's always caused by some earlier error.

This comment has been minimized.

Sign in to view

Copy link
@davidrenne

davidrenne May 16, 2017

Contributor

Awesome collaborating with you! Lol. I love swallowing the error term. React will eat the error but I think the stack will eat and barf out the error elsewhere I am sure. I'll send the pull request soon
}

performance.clearMarks(markName);
performance.clearMeasures(measurementName);
}

var ReactDebugTool = {
addHook(hook) {
hooks.push(hook);
Expand Down Expand Up @@ -244,11 +301,13 @@ var ReactDebugTool = {
onBeginLifeCycleTimer(debugID, timerType) {
checkDebugID(debugID);
emitEvent('onBeginLifeCycleTimer', debugID, timerType);
markBegin(debugID, timerType);
beginLifeCycleTimer(debugID, timerType);
},
onEndLifeCycleTimer(debugID, timerType) {
checkDebugID(debugID);
endLifeCycleTimer(debugID, timerType);
markEnd(debugID, timerType);
emitEvent('onEndLifeCycleTimer', debugID, timerType);
},
onBeginProcessingChildContext() {
Expand All @@ -273,25 +332,31 @@ var ReactDebugTool = {
checkDebugID(debugID);
checkDebugID(parentDebugID, true);
emitEvent('onBeforeMountComponent', debugID, element, parentDebugID);
markBegin(debugID, 'mount');
},
onMountComponent(debugID) {
checkDebugID(debugID);
markEnd(debugID, 'mount');
emitEvent('onMountComponent', debugID);
},
onBeforeUpdateComponent(debugID, element) {
checkDebugID(debugID);
emitEvent('onBeforeUpdateComponent', debugID, element);
markBegin(debugID, 'update');
},
onUpdateComponent(debugID) {
checkDebugID(debugID);
markEnd(debugID, 'update');
emitEvent('onUpdateComponent', debugID);
},
onBeforeUnmountComponent(debugID) {
checkDebugID(debugID);
emitEvent('onBeforeUnmountComponent', debugID);
markBegin(debugID, 'unmount');
},
onUnmountComponent(debugID) {
checkDebugID(debugID);
markEnd(debugID, 'unmount');
emitEvent('onUnmountComponent', debugID);
},
onTestEvent() {
Expand Down

0 comments on commit cbc4508

Please sign in to comment.