Skip to content

Profiler RFC #51

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.

Sign up for GitHub

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

Merged
bvaughn merged 12 commits into from
Jul 25, 2019
Merged

Profiler RFC #51

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
317e02a
Profiler RFC
May 22, 2018
dfe022f
Unresolved questions
May 22, 2018
58afaa1
Formatting nits
May 22, 2018
b529613
Organizational tweask
May 22, 2018
1475772
Added an async caveat about actual time
May 22, 2018
7cb55e6
Typofix
May 22, 2018
3e9edf1
Added clarfiication note about how <Profiler> works in production bundle
May 22, 2018
6062c52
Typofix
Jun 9, 2018
b1834f9
Renamed actualTime -> actualDuration and baseTime -> baseDuration
Aug 3, 2018
c263445
Updated note about Profiler experimental release
Aug 3, 2018
a5c38f4
Added interaction-tracking proposal to RFC
Aug 3, 2018
009a0b4
Udpated RFC
Jul 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Formatting nits
  • Loading branch information
Brian Vaughn committed May 22, 2018
commit 58afaa12e08dc174f08edeb7c3d434bb5358c14e
23 changes: 8 additions & 15 deletions text/0000-profiler.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,14 @@ Timing measurements should also be significantly lighter weight than the current
The `onRender` callback is called each time a component within the `Profiler` renders. It receives the following parameters:
```js
function onRenderCallback(
id,
phase,
actualTime,
baseTime,
startTime,
commitTime
) {
// Aggregate or log render timings
id: string,
phaseL "mount" | "update",
actualTime: number,
baseTime: number,
startTime: number,
commitTime: number
): void {
Copy link

@j-f1 j-f1 May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not pass all of this as a single object so people can pick out the keys they need?

Copy link
Collaborator Author

@bvaughn bvaughn May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasonable question!

I'd say the reasons are that I'm following precedent (we don't pass named parameters anywhere else that I can think of off the top of my head) and avoiding allocating a wrapper Object during commit.

I'd be interested to hear what others think about this aspect.

Copy link

@jamesreggio jamesreggio May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote for an object, despite the fact that it breaks with existing React API precedent.

The order of these timing arguments is going to be tough to memorize, and I can imagine only being interested in a subset of them. Using an object also enables you to add additional timing data down the road.

I'd view it as somewhat analogous to an event object, which has a variety of keys, only some of which are of interest for any given listener.

Copy link
Member

@gaearon gaearon May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you need to memorize it? I imagine you'd only use Profiler in a few places in the app, and each time could consult the docs.

The need to avoid allocations is pretty important because adding GC pressure can skew the profiling results.

Copy link
Collaborator Author

@bvaughn bvaughn May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if you use Profiler in more than one place, the callback you pass it is likely shared- (this is why the id parameter exists)- so you would only need to write these params (in the correct order) in a single place.

Copy link
Collaborator Author

@bvaughn bvaughn May 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this concern makes sense. And I agree that we wouldn't be allocating too many new objects for this, because it would only be one per Profiler per commit. I was just sharing rationale for why it is currently the way it is.

Copy link

@alvaropinot alvaropinot Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my personal opinion here, but an object looks good from my point of view as long functions with more that 2/3 args are always hard to remember, you tend to start adding null for the values that you might not want to use, I'm looking at you JSON.stringify(foo, null, 2) 😅 , you also need to remember the order and it's harder to refactor as you impact anyone already using that order.

Plus with the actual syntax for destructuring the function signature looks pretty much the same but with curly braces 😁, the best of two worlds!

onRenderCallback({ id, phase, actualTime, baseTime, startTime, commitTime })

vs

onRenderCallback(id, phase, actualTime, baseTime, startTime, commitTime)

Copy link

@jamesreggio jamesreggio Jun 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the Profiling API is out in the 16.4.1 release, I assume you decided to take no action on this?

Copy link
Collaborator Author

@bvaughn bvaughn Jun 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unstable_Profiler component was introduced in 16.4.0. The only thing that's new in 16.4.1 is a production+profiling build.

Unstable APIs can change. We haven't decided one way or another. This is kind of an open thread for discussion.

Copy link
Collaborator Author

@bvaughn bvaughn Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just circling back on this particular thread. Sebastian and I chatted about this yesterday, and we've decided to avoid named parameters because the overhead of the wrapper objects (however small each individual one is) will add up in larger applications.

// Aggregate or log render timings...
}

```
Expand All @@ -101,13 +101,6 @@ Start time isn't just the commit time less the "actual" time, because in async r
#### `commitTime: number`
Commit time could be roughly determined using e.g. `performance.now()` within the `onRender` callback, but multiple `Profiler` components would end up with slightly different times for a single commit. Instead, an explicit time is provided (shared between all `Profiler`s in the commit) enabling them to be grouped if desirable.

---

Also see the following implementation PRs:
* [facebook/react/pull/12745](https://github.com/facebook/react/pull/12745): Add `Profiler` component and tests
* [facebook/react/pull/12852](https://github.com/facebook/react/pull/12852): Add start and stop time params
* [facebook/react/pull/12886](https://github.com/facebook/react/pull/12886): Add production + profiling bundle for ReactDOM and ReactNative

# Drawbacks

Overuse of this component might negatively impact application performance.
Expand Down