Skip to content
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.

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

Remove thread-loader #5170

Merged
merged 2 commits into from
Sep 29, 2018
Merged

Remove thread-loader #5170

merged 2 commits into from
Sep 29, 2018

Conversation

Timer
Copy link
Contributor

@Timer Timer commented Sep 29, 2018

After the cache is warm, it is possible for thread-loader to hurt speed.
thread-loader also has leaky interaction with its workers, giving us some junk output here and there.

This will hurt cold boots/builds the most, but with our cache rules it should be very seldom someone needs to clear their cache (if ever).

Per #5138 (comment), future work is to write our own worker that handles caching and parallelization.


Fixes #5138

After the cache is warm, it is possible for thread-loader to hurt speed
@gaearon
Copy link
Contributor

gaearon commented Sep 29, 2018

Do we have a clear idea of how much slower to cold build is in DEV or PROD?

@Timer
Copy link
Contributor Author

Timer commented Sep 29, 2018

Per previous thread, prod build:

Cold:
Thread loader disabled: Done in 71.03s.
Thread loader enabled: Done in 59.11s

Size of application for scale:

File sizes after gzip:

  387.99 KB  build/static/js/6.c8fffdff.chunk.js
  286.88 KB  build/static/js/5.057175f6.chunk.js
  273.24 KB  build/static/js/3.b13196e5.chunk.js
  137.74 KB  build/static/js/4.21e358cd.chunk.js
  62.42 KB   build/static/js/main.f8e7c995.chunk.js
  21.19 KB   build/static/css/3.07d6ad7e.chunk.css
  1.06 KB    build/static/css/main.6781b2da.chunk.css
  210 B      build/static/js/1.a6f64de4.chunk.js
  145 B      build/static/js/2.f7e78bdb.chunk.js

@gaearon
Copy link
Contributor

gaearon commented Sep 29, 2018

That's for prod right? What about dev boot?

Seems like snapshots need updating

@Timer
Copy link
Contributor Author

Timer commented Sep 29, 2018

Dev, cold:

joes-mbp:website joe$ rm -rf node_modules/.cache/
joes-mbp:website joe$ time BROWSER=none yarn start --smoke-test | cat # thread-loader on
yarn run v1.10.1
real    0m27.333s
user    0m20.362s
sys     0m1.676s
joes-mbp:website joe$
joes-mbp:website joe$ rm -rf node_modules/.cache/
joes-mbp:website joe$ time BROWSER=none yarn start --smoke-test | cat # thread-loader off
yarn run v1.10.1
real    0m39.025s
user    0m58.762s
sys     0m3.307s
joes-mbp:website joe$

Dev, warm:

joes-mbp:website joe$ time BROWSER=none yarn start --smoke-test | cat # thread-loader on
yarn run v1.10.1
real    0m14.610s
user    0m18.740s
sys     0m1.695s
joes-mbp:website joe$
joes-mbp:website joe$ time BROWSER=none yarn start --smoke-test | cat # thread-loader off
yarn run v1.10.1
real    0m14.019s
user    0m19.680s
sys     0m1.621s
joes-mbp:website joe$

@Timer
Copy link
Contributor Author

Timer commented Sep 29, 2018

Cold dev boot suffers, but appear on-par/faster once cache is warm.

I suspect our dev rebuilds will be much quicker with no thread-loader, too (skipping IPC for 1 file change).

@Timer
Copy link
Contributor Author

Timer commented Sep 29, 2018

Disabled broken test for now, will fix in follow up PR with the rest of output & tests.

@iansu
Copy link
Contributor

iansu commented Sep 29, 2018

I'm in favour of this. Thread loader has ended up being the cause of a number of issues.

@Timer Timer merged commit 5abff64 into facebook:master Sep 29, 2018
@Timer Timer deleted the drop-thread-loader branch September 29, 2018 22:30
@miraage
Copy link

miraage commented Sep 30, 2018

We had an issue with worker-loader + thread-loader as well

zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
* Remove thread-loader
After the cache is warm, it is possible for thread-loader to hurt speed

* Disable babel output formatting
@ersel
Copy link

ersel commented Oct 2, 2018

This was breaking our builds on CI environment.

As CircleCI agents seemed to have 32 core CPUs but not enough RAM to handle the number of threads spawn by thread-loader. Glad it was removed. 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants