1224765 - Upgrade to React 0.14.6 and include dev & prod version

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Screen Shot 2015-11-13 at 2.29.57 PM.png

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Screen Shot 2015-11-13 at 3.49.21 PM.png

Comment 3

[James Long (:jlongster)]

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.

Comment 4

[James Long (:jlongster)]

Forgot to be explicit about this: that would mean there is only one react.js file.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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).

Comment 6

[James Long (:jlongster)]

Attachment: 1224765.patch

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.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 8

[James Long (:jlongster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=63f38810092d

Comment 9

[James Long (:jlongster)]

New run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e86eec4d5b56

Comment 10

[James Long (:jlongster)]

still some jetpack failures (because of my path resolution tweak), think I fixed it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2db28640ca67

Comment 11

[James Long (:jlongster)]

Attachment: 1224765.patch

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

Comment 12

[James Long (:jlongster)]

Attachment: 1224765.patch

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

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9107019e69c3

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9107019e69c3

Comment 15

[James Long (:jlongster)]

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]:

Comment 16

[Sylvestre Ledru [:Sylvestre]]

(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?!

Comment 17

[James Long (:jlongster)]

(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

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8709526 [details] [diff] [review]
1224765.patch

OK, let's take it then!

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d16d1e082056

Comment 20

[Mayur Patil]

[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