Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ReactNativeOperationHistoryDevtool to track native operations #6612

Merged
merged 1 commit into from
Apr 27, 2016
Merged

Add ReactNativeOperationHistoryDevtool to track native operations #6612

merged 1 commit into from
Apr 27, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Apr 25, 2016

This is extracted from #6046 and implements a new devtool that tracks native operations.

@gaearon gaearon changed the title (WIP) Instrument native operations (WIP) Add ReactNativeOperationHistoryDevtool to track native operations Apr 25, 2016
@ghost
Copy link

ghost commented Apr 26, 2016

@gaearon updated the pull request.

2 similar comments
@ghost
Copy link

ghost commented Apr 26, 2016

@gaearon updated the pull request.

@ghost
Copy link

ghost commented Apr 26, 2016

@gaearon updated the pull request.

@gaearon gaearon changed the title (WIP) Add ReactNativeOperationHistoryDevtool to track native operations Add ReactNativeOperationHistoryDevtool to track native operations Apr 26, 2016
@ghost
Copy link

ghost commented Apr 26, 2016

@gaearon updated the pull request.

@sebmarkbage
Copy link
Collaborator

Hm. What would you think about just passing the new "props" object instead of onNativeOperation or onSetValueForProperty? Like onPropsChanged(debugID, props). Would that be sufficient?

How the native "props" get applied is a fast moving implementation detail of each renderer.

@sebmarkbage
Copy link
Collaborator

Come to think of it, why isn't props already an argument to onUpdateComponent and onMountComponent? Wouldn't you need to get the props to build a debug tree for things like the Chrome devtools? Whatever API gets the props for a composite component could get them for the native component.

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 27, 2016

Yeah, it should be an argument, I just didn’t have a use case yet in ReactPerf.

I was shooting for similar output as existing ReactPerf whose printOperations() print actual DOM work. Tracking new props is not quite equivalent. For example, update style currently only fires for changes individual values but if we were to fire events for any changed native props, the log would be spammed with update styles that have no effect on the render of every component containing them.

To make it useful, we would have to make the devtool aware of the fact that unlike any other native props, style itself needs to be shallowly unequal to the previous version, which mean we would have to keep it around and make the devtool aware of the special nature of style in the DOM renderer which doesn’t seem nice.

Spamming the log is not just bad because it becomes less useful. It is also bad because it makes printWasted() broken. If every style change is counted as a native operation, printWasted() will never consider components with inline styles wasted because they always seem to produce native operations. Maybe this isn’t the best heuristic but this is the single method people use the most and we can’t break it.

Indeed, native operations may change any day. This is actually exactly how this devtool is supposed to work. It reports whatever the renderer tells it, the names of everts are not set in stone. When DOM renderer changes to do things in a different way, it is free to fire different native operations in the places where it interfaces with the DOM to give the user a picture of what is going on.

@sebmarkbage
Copy link
Collaborator

printWasted could do an deepEqual test. One thing I wanted to try is never diffing native components, not store the previous props, and always override the native ones. Because the DOM calls in modern browsers now have little overhead and is effectively diffed inside the DOM anyway. The bulk of the work has already been done because lack of shouldComponentUpdate bailing out.

Basically when I turn that on, any of these assumptions in the tooling would immediately break and I'd have to go fix it.

I think there is a false assumption that DOM operations are heavy and therefore we overfocus on this level of the instrumentation. ReactPerf printing these operations is probably an artifact of that.

I'll accept for equivalence but I think we should change the heuristic and logged data to something more representative.

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 27, 2016

I'll accept for equivalence but I think we should change the heuristic and logged data to something more representative.

👍

I filed it as #6632 and will come back to it after the old ReactPerf is gone.
For now let’s shoot for feature parity.

@gaearon gaearon added this to the 15.x milestone Apr 27, 2016
@gaearon gaearon merged commit 3bdf09e into facebook:master Apr 27, 2016
@gaearon gaearon deleted the instrumentation-new-operations branch April 27, 2016 18:03
zpao pushed a commit that referenced this pull request May 10, 2016
Add ReactNativeOperationHistoryDevtool to track native operations
(cherry picked from commit 3bdf09e)
@zpao zpao modified the milestones: 15.1.0, 15.y.0 May 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants