Closed Bug 1224765 Opened 5 years ago Closed 5 years ago

Upgrade to React 0.14.6 and include dev & prod version

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox45 fixed, firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox45 --- fixed
firefox46 --- fixed

People

(Reporter: fitzgen, Assigned: jlong)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Seeing around 7% of cpu time and ~27% of allocations (which is putting extra pressure on the GC, which is also showing up in profiles) in react's "warning" method **even though it isn't actually emitting warnings**. In production builds, the warning method is a noop.

We need to figure out how to use production builds in non-DEBUG/non-DEBUG_JS_MODULES builds for real.
It's not just that it's a noop, in React they have `ENV !== "production"` everywhere, even when they call methods like `invariant`, so that the arguments aren't even evaluated. When minifying if `"production !== "production"` that code will just be totally strip out by dead code elimination.

Because we didn't minify our production build, there were literal `"production" !== "production"` checks but I think that should be good enough for us.

I've actually been thinking that we could probably just do a custom build of React that dynamically checks the environment. So at the top of the file it would have something like:

const ENVIRONMENT = (DevToolsUtils.testing || AppConstants.DEBUG_JS_MODULES) ? "development" : "production"

or something like that. And then we would replace the checks with `ENVIRONMENT !== "production"`. I think it would be easy to replace that constant as a build step with webpack.

I can't think of a problem with making it a dynamic check.
Forgot to be explicit about this: that would mean there is only one react.js file.
Don't really care whether we end up with one react.js file or two; unclear to me whether that is less maintenance overhead than not modifying vendor sources.

`ENV != "production"` checks should be fine, I don't think we need to minify or anything like that. The JIT can boil that kind of thing away easily (and it might actually be one of the few optimizations that happen when emitting bytecodes).
Assignee: nobody → jlong
Attached patch 1224765.patch (obsolete) — Splinter Review
So what I ended up doing was manually including TestUtils unconditionally. It's a super simple manual change when doing our custom build of React and I updated the upgrade instructions for doing so.

This adds back the dynamic check for using dev/prod versions of React, as well as adds the prod version and upgrades to 0.14.6.

We also need to fix a check in the loader because it would match "vendor/react-dom.js" with a patch added to the loader of just "vendor/react", while it really should just be matching on path segments.
Attachment #8707653 - Flags: review?(nfitzgerald)
Comment on attachment 8707653 [details] [diff] [review]
1224765.patch

Review of attachment 8707653 [details] [diff] [review]:
-----------------------------------------------------------------

Great! This was badly needed, thanks!

::: devtools/shared/DevToolsUtils.js
@@ +523,5 @@
> + * Determines if the current environment is in debug mode for
> + * JavaScript sources.
> + */
> +exports.isDebugMode = function() {
> +  return AppConstants.DEBUG || AppConstants.DEBUG_JS_MODULES || this.testing

I think the `this` is wrong here.
Attachment #8707653 - Flags: review?(nfitzgerald) → review+
Summary: figure out some way to use production react → Upgrade to React 0.14.6 and include dev & prod version
still some jetpack failures (because of my path resolution tweak), think I fixed it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2db28640ca67
Attached patch 1224765.patch (obsolete) — Splinter Review
Latest patch, with a more expansive try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80710ab3f0f6. will push later today if that looks good
Attached patch 1224765.patchSplinter Review
Oops, needed to also include react-dev.js when MOZ_DEBUG is set to true because we automatically switch to it in regular DEBUG mode as well. (debug tests couldn't find the file)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=346f8c6fef3b
Attachment #8707653 - Attachment is obsolete: true
Attachment #8709457 - Attachment is obsolete: true
Blocks: 1238881
https://hg.mozilla.org/mozilla-central/rev/9107019e69c3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Comment on attachment 8709526 [details] [diff] [review]
1224765.patch

Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: memory tool is a little slower
[Describe test coverage new/current, TreeHerder]: no new tests needed, current code that uses React covers this
[Risks and why]: not much risk
[String/UUID change made/needed]:
Attachment #8709526 - Flags: approval-mozilla-aurora?
(In reply to James Long (:jlongster) from comment #15)
> [Risks and why]: not much risk
Even if it is a new upstream release, why do you think it is not risky? 
It baked only a day in m-c, it could have some unexpected side effect?!
Flags: needinfo?(jlong)
(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> (In reply to James Long (:jlongster) from comment #15)
> > [Risks and why]: not much risk
> Even if it is a new upstream release, why do you think it is not risky? 
> It baked only a day in m-c, it could have some unexpected side effect?!

If you're thinking mostly of the upgrade, we were on 0.14.0 before so this new version is just bugfixes and nothing should change. The main thing this bug did was add the production version, which is semantically equivalent but avoids a lot of checks so it runs a bit faster.

I think this is very little risk, and we can wait until tomorrow to actually uplift so it's on m-c for a little more time? I was hoping to get this in before the merge.

Also, it's very little risk in that it's entirely devtools-related, and even so, only the memory tool right now is using React (as well as a few other places like JSON Viewer), but they are all really infrequently used tools. There's no risk for Firefox outside of devtools, or even our main tools like console/debugger/etc
Flags: needinfo?(jlong)
Comment on attachment 8709526 [details] [diff] [review]
1224765.patch

OK, let's take it then!
Attachment #8709526 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name 			Firefox
Version 		46.0b9
Build ID 		20160322075646
Update Channel 	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                      Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.