Added production profiling bundle type #12886
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Details of bundled changes.Comparing: bfb12eb...cd39422 react-dom
react-native-renderer
Generated by 🚫 dangerJS |
Merged
4 tasks
bvaughn
force-pushed
the
profiling-build
branch
2 times, most recently
from
a50b70c
to
812beec
Compare
|
Rebaaaaased |
nhunzaker
approved these changes
Jun 5, 2018
There was a problem hiding this comment.
This looks good to me.
At some point, I'd like to figure out how to do a limited build set to speed up turn around time when testing DOM fixtures. We only need the UMD dev (edit: dom and core) builds in that case.
yarn build "dom-client, core" definitely speeds things up, and some times I just comment out the other build 😸.
Still at some point I'd like to look into adding a more fine grained filter (also maybe running Rollup in watch mode).
|
Thanks Nate! 😄 Have you tried? yarn build dom-client,core --type=UMD |
|
Dang. I need to put this in the DOM fixture readme. Thanks! |
|
Any reviewers other than Nate want to take a look and weigh in? |
gaearon
reviewed
Jun 10, 2018
packages/shared/ReactFeatureFlags.js
Outdated
| @@ -37,7 +37,8 @@ export const warnAboutDeprecatedLifecycles = false; | |||
| export const warnAboutLegacyContextAPI = false; | |||
|
|
|||
| // Gather advanced timing metrics for Profiler subtrees. | |||
| export const enableProfilerTimer = __DEV__; | |||
| export const enableProfilerTimer = | |||
| __DEV__ || process.env.REACT_ENABLE_PROFILING === true; | |||
There was a problem hiding this comment.
Does it need to be on process.env, as opposed to another global?
My understanding is that this flag always gets replaced in build process. So we don’t actually let people use it, do we? Not unless they import our bundle directly. At that point we might as well call it __PROFILE__ to better indicate its internal only.
There was a problem hiding this comment.
In case it’s not clear I mean that we didn’t change npm entry point so there’s no way external consumers can set this variable.
There was a problem hiding this comment.
I originally named it __PROFILER__ but backed it out in 812beec because of Flow. I don't feel strongly about that change though. Happy to change it back and just add a Flow declaration.
Edit Done.
There was a problem hiding this comment.
I'm mostly curious about your intention. What is your plan for people to use this in CJS mode?
There was a problem hiding this comment.
Intention wasn't to let people override this value at runtime, so you're right that using process.env was probably misleading or confusing.
What is your plan for people to use this in CJS mode?
Haven't thought a lot about this, but I was assuming people would alias react (and renderer) imports for production builds if they wanted to use profiling capabilities.
added 6 commits
June 10, 2018 07:56
|
I think at some point we discussed allowing to opt into profiling bundle with an env variable (instead of aliasing). But I haven’t thought about whether it would break dead code elimination or not. |
My interpretation/recollection of that idea was that a query parameter and/or env variable could be used to determine which version of react (and renderer) was required- production or profiling. Including profiling capabilities in the production build would impact DCE for places like |
|
I didn’t propose to include it in the production build, but to do a switch of the bundle at the top level (just like we do with development/production). But I’m worried that there might be no way to have a separate env variable without breaking dead code elimination for people who don’t set it at all. |
|
I guess I misunderstood your initial suggestion. I don't know that we need to decide that aspect in order to move on with this PR. Even just having a production+profiling bundle that people could alias to themselves would be a nice incremental change. |
gaearon
approved these changes
Jun 11, 2018
|
Before merging can you please make sure once again this didn't affect existing DEV/PROD bundles now that you've changed it to use |
|
Do we want people aliasing though? We wanted to treat the nested bundles as opaque so that we can do things like inlining it into the main bundle to avoid packaging woes. Seems like mixed messaging. It's also not a very scalable solution because we might want to move things to the react package and later the scheduler package. All of those have to be aliased and things might break in weird ways when you get it wrong. A possible solution could be to embed something like |
|
That's additive though. We can still merge this for initial experimentation. Then when we announce |
|
Is there any concern that adding an environment variable could lead to component ecosystem around I don't think that decision should block this PR, but that's my only concern with using an environment variable. |
|
I don't see why that would be bad. I'm more concerned that DCE might not work at all for some people because they don't explicitly have this variable in their configs. |
Yup! I'll rebase and do one more comparison. |
|
Comparing the new 2829a2830
> this.treeBaseTime = this.selfBaseTime = this.actualStartTime = this.actualDuration = 0;
2843c2844,2846
< (d.lastEffect = null));
---
> (d.lastEffect = null),
> (d.actualDuration = 0),
> (d.actualStartTime = 0));
2851a2855,2856
> d.selfBaseTime = a.selfBaseTime;
> d.treeBaseTime = a.treeBaseTime;
3204c3209,3219
< function lg(a, b, c) {
---
> var kg = 0,
> lg = 0,
> mg = 0;
> function ng(a) {
> a.actualDuration = L() - mg - a.actualDuration;
> }
> function pg() {
> 0 < lg && ((mg += L() - lg), (lg = 0));
> }
> var qg = -1;
> function rg(a, b, c) {
3827c3846,3849
< function Rg(a, b, c) {
---
> function Xg(a, b, c) {
> b.mode & 4 &&
> ((b.actualDuration = L() - b.actualDuration - mg),
> (b.actualStartTime = L()));
4451c4476
< null !== f && ((c[Ma] = d), Xe(c, f, e, a, d));
---
> null !== f && ((c[Ma] = d), Ye(c, f, e, a, d));
4460a4486,4494
> c = b.memoizedProps.onRender;
> c(
> b.memoizedProps.id,
> null === a ? "mount" : "update",
> b.actualDuration,
> b.treeBaseTime,
> b.actualStartTime,
> kg
> );
4613,4617c4649,4663
< for (g = e.child; null !== g; )
< 0 !== g.expirationTime &&
< (0 === f || f > g.expirationTime) &&
< (f = g.expirationTime),
< (g = g.sibling);
---
> if (e.mode & 4) {
> g = e.selfBaseTime;
> for (var h = e.child; null !== h; )
> (g += h.treeBaseTime),
> 0 !== h.expirationTime &&
> (0 === f || f > h.expirationTime) &&
> (f = h.expirationTime),
> (h = h.sibling);
> e.treeBaseTime = g;
> } else
> for (g = e.child; null !== g; )
> 0 !== g.expirationTime &&
> (0 === f || f > g.expirationTime) &&
> (f = g.expirationTime),
> (g = g.sibling);
4650,4653c4696,4702
< function xh(a) {
< var b = Rg(a.alternate, a, T);
< null === b && (b = wh(a));
< ec.current = null;
---
> function Dh(a) {
> var b = a.alternate;
> a.mode & 4 && (qg = L());
> b = Xg(b, a, T);
> a.mode & 4 && (-1 !== qg && (a.selfBaseTime = L() - qg), (qg = -1));
> null === b && (b = Ch(a));
> fc.current = null; |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a new "profiling" target to ReactDOM (cjs) and ReactNative (oss) bundles. This is a production build (
process.env.NODE_ENV === "production") but has the profiling timing flag enabled (via a new env variableprocess.env.REACT_ENABLE_PROFILING).I don't believe that other bundles need this new build type, but it would be easy enough to add if we decided it did.