Closed Bug 1224765 Opened 5 years ago Closed 5 years ago
Upgrade to React 0
.14 .6 and include dev & prod version
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).
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)
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
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
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 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?!
(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 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
You need to log in before you can comment on or make changes to this bug.