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

Performance issues #14

Closed
BowlingX opened this issue Jun 15, 2017 · 21 comments
Closed

Performance issues #14

BowlingX opened this issue Jun 15, 2017 · 21 comments

Comments

@BowlingX
Copy link
Contributor

Hi,
thank you for this great plugin. I'm running to some performance issues with a big project.
Due the multiple bundles generated (one with and one without css), the build time increases significant (at least double in production with minification) in comparision with just extract-css-webpack-plugin . It also seems to me, that the no_css get's generated when there is no css imported at all.

In my case it goes from 6 minutes to 16 Minutes.

@faceyspacey
Copy link
Owner

Wow that's a big build.

A pr just came in yesterday to turn off the no_css chunks all together. Would that work for you or are you saying that some of your chunks have css and some not?

I don't suppose u could put some timers in node_modules/extract-css-chunks-webpack-plugin/index.js (and maybe loader.js) and figure out precisely what the problem is. It's either at the bottom of index or the bottom of loader.

Also checkout the pr. I don't wanna merge it until I'm 100% sure about the option name. But try turning off no_css chunks like in the PR and see if we still have the problem. That way we can know whether we can fully blame it on those extra chunks. There's a chance it's not. It's not the only thing I changed.

@BowlingX
Copy link
Contributor Author

I created a pull-request to skip duplicating chunks that have no css. In my case it goes down to around 13 minutes now (always depending on the project I guess). Fully Skipping the duplicated chunks reduces it to 8 minutes, so I think this is the main issue.

@BowlingX
Copy link
Contributor Author

What if the css stuff get's removed after optimizing instead of before ?

@BowlingX
Copy link
Contributor Author

What also drains more performance is that all chunks / css files get extracted and therefore the style-loader (that is not fast in general) adds additional load on top. I would suggest to restore the configuration initialChunks only or allChunks.

@BowlingX
Copy link
Contributor Author

And I think we should skip the generation of no_css files for non initial chunks (or make it configurable).

@BowlingX
Copy link
Contributor Author

I actually would expect to behave like this:

app.js
   - basicStyles.css
   - routes: 
          - homepage.js (async)
             - homepageStyle.css
          - contact.js (async)
             - contactStyles.css

yields: (if we would create one entry per route)

homepage.bundle.js (app.js + homepage.js) no css
homepage.bundle.css (app.css + homepage.css only)

contact.bundle.js (app.js + contact.js) no css
contact.bundle.css (app.css + contact.css only)

Async part:
0.js (only homepage js + css)
1.js (only contact js + css)

@faceyspacey
Copy link
Owner

faceyspacey commented Jun 17, 2017

What if the css stuff get's removed after optimizing instead of before ?

Maybe, but I think at a certain point it becomes hard to parse the minified version of what needs to be removed.

style-loader (that is not fast in general) adds additional load on top

...Here's how I use style-loader:

https://github.com/faceyspacey/extract-css-chunks-webpack-plugin/blob/master/loader.js#L139
So i dont think it's style-loader that's slow. I just use it to inject it on the client. What's slow is that i "paste" the css into all your javascript modules (like I assume style-loader would otherwise do). I'm just saying--maybe there's a faster way to do what I did right there. But I'm not doing much besides string interpolation.

I would suggest to restore the configuration initialChunks only or allChunks.

but then why not just use the original EWTP? ...i usually don't have setups with multiple named chunks, and just rely on dynamic chunks, but I think I know what you want (based on your PR):

  • automatic detection of whether bundles even have CSS, and if not, dont create a no_css.js file.
  • make additional css files configurable as per this PR.

Does combining those 2 PRs solve the problem? Do we need deeper specification options for toggling no_css.js creation. Like, do we let u specify whether it's on or off at both the named chunk level and dynamic chunk level?

And if so, story over?

@faceyspacey
Copy link
Owner

faceyspacey commented Jun 17, 2017

also, let me know if u have any input on the noCssChunks option name, etc. as per @igogrek 's PR.

@faceyspacey
Copy link
Owner

...unfortunately, we also have this issue:

faceyspacey/universal-demo#1

I asked a Webpack team member about how to solve it here (with no response yet):
#3 (comment)

if you have any ideas about that one, it would really help this out. I plan to more publicly release/promote just this plugin once that's solved. And urs and @igogrek 's also make a whole lotta sense to have before launching.

@faceyspacey
Copy link
Owner

here's a short private/unpublished launch article im releasing in a few days:

https://medium.com/@faceyspacey/extract-css-chunks-webpack-plugin-chunkify-your-css-a-few-things-you-need-45cb474cc332

feedback is much appreciated. trying to make this as obvious to people as possible. anything i should strip away or add? or say better?

...guys help me get this (as in the package in general) ready for prime time. we all know its very needed.

@BowlingX
Copy link
Contributor Author

BowlingX commented Jun 17, 2017

style-loader (that is not fast in general) adds additional load on top

Sorry, I meant the css-loader, see this issue here: webpack-contrib/css-loader#124. So in general, I would prefer to have as less as possible duplicated work.

but then why not just use the original EWTP? ...i usually don't have setups with multiple named
chunks, and just rely on dynamic chunks, but I think I know what you want (based on your PR):

good question :). the EWTP plugin does not load the additional styles async, that's how I found your repository (trough this issue: webpack-contrib/extract-text-webpack-plugin#120)

I think however, that this is a bug and I'm constructing a test case right now.

@BowlingX
Copy link
Contributor Author

I created a pull request here, that addresses the issue and shows a test case webpack-contrib/extract-text-webpack-plugin#546

@faceyspacey
Copy link
Owner

faceyspacey commented Jun 17, 2017

...ok, but didnt u solve your problem here with your PR? If the other PR is combined, doesn't that solve all the problems you're interested in?

@BowlingX
Copy link
Contributor Author

BowlingX commented Jun 18, 2017

I was trying to find a better solution for removing the stuff from the initial chunk, without reiterating and removing the styles with the regex. I found something that actually keeps the style-loader in the async chunks and I just apply the extraction to the initial chunk. It was a bit hard to get my head around it due the complex structure and not much documentation, but I think it's working properly.

All tests also still work and it does not require manual adding any hot reload stuff (it will keep everything from the style-loader).

I did not made any more modifications that you added (for example extracting the async css files and more). Maybe this is a good base to extend your version.

here is the pull-request: webpack-contrib/extract-text-webpack-plugin#546

@faceyspacey
Copy link
Owner

faceyspacey commented Jun 18, 2017

I think it's a great one. I don't suppose you can commit it here. That other repo is very slow going with pull requests or this and the HMR code (which i got from another never-merged PR) would be in the main repo already.

My long term hope is to get all this code back in that repo. My thinking is if we can prove the value and that people want it here, it will be more likely to be merged back into ETWP. I've been in contact with the Webpack people about this. It seems if we prove the value, we will increase priority in getting this merged. They got tons of code/plugins/etc after all. Their time is limited. But, they are getting more focused on code-splitting and SSR (as u can see with their new "magic @comments" feature). So it makes more and more sense for them to get this stuff right.


I'm working on this today, and aiming to publish the above release article tomorrow. I'll give you lots of credit. If ur around, we can work together.


If you have the time, why dont u merge all the same code into a new PR here, and remove as much of my code as u need to make it work, and then I'll bring back the async css stuff. Let me know if you have any ideas on how to do that better as well. I like your improvements. Performance was one of the main things I was worried about but didn't have time to test. If we can figure out how to make the css files stuff faster too, that would take this plugin to the next level.

@BowlingX
Copy link
Contributor Author

Sorry I did not read your message yesterday anymore. It was 3AM already in my timezone.
My pull-request covers the async part already and also HMR (at least for the async chunks). The thing that is missing is extracting and generating css files from the async bundles (should be configurable properly).

I'm also removing all css from the entry bundle. I think that, if hot reload is enabled, the plugin should be disabled. If the stylesheet is loaded additionally to the injected ones and you remove css rules, it will still load them trough the static css file.

I'm not sure if my modifications allow everything that you are doing with webpack-flush-chunks for example.

I can create a pull request with my changes. I think it's also importend to let the tests run on the CI.

@BowlingX
Copy link
Contributor Author

Please check the updated request: #16

@faceyspacey
Copy link
Owner

faceyspacey commented Jun 28, 2017

@BowlingX quick question for you: how did you ever figure out what the hell this plugin is doing? Perhaps ur a better coder than I have, have done a lot more work on webpack plugins or build tools in general, but this code isn't commented at all. And there's no documentation besides that one webpack compilation lifecycle page. It's impossible to figure out unless you have spent tremendous time with perhaps other plugins.

I write this because I really wanna contribute to ur EWTP PR the real correct way to create async chunks, but I can't figure it out. I mean i can make it, cuz obviously i did it here. But before I do it again, i want to fully understand what it's doing. I don't suppose you can add lots of comments to your EWTP PR. It's impenetrable for anyone besides sokra and apparently you.

@BowlingX
Copy link
Contributor Author

@BowlingX quick question for you: how did you ever figure out what the hell this plugin is doing? Perhaps ur a better coder than I have, have done a lot more work on webpack plugins or build tools in general, but this code isn't commented at all. And there's no documentation besides that one webpack compilation lifecycle page. It's impossible to figure out unless you have spent tremendous time with perhaps other plugins.

Took me 2 days to figure it out actually. I started with a test and a small use-case what I expect from being generated. This way I could quickly test any changes I made to the code. Then I basically just dug trough it, even used a remote debugger (check node --inspect) (this way you can set breakpoints). First I had to understand when what code was executed. I wrote small plugins before, but not really complex stuff. What also helped me was this: https://webpack.github.io/docs/plugins.html. It describes the lifecycle hooks you can react to. This way I discovered I could manually remove modules from the compilation.

I must say that I could not find any documentation regarding the webpack internal APIs, so I literally console.logged the output of the objects (or used the remote debugger to inspect what method's it's provides), read the webpack source and then just experimented with it.

@faceyspacey
Copy link
Owner

i guess i've just been lazy. im a big fan of debugging, but took the route of avoiding setting that up, as i've never debugged a build tool.

any tips on the right way to do dynamic import extraction?

@faceyspacey
Copy link
Owner

faceyspacey commented Jul 7, 2017

I released the version I talked about with the dual imports via a babel plugin:

https://medium.com/faceyspacey/webpacks-import-will-soon-fetch-js-css-here-s-how-you-do-it-today-4eb5b4929852

It works fine with 3.0. It's still the old codebase with a bit of some of what you did left lingering. As you probably figured out, Sokra's "master plan" involves dual imports too. I just happened to have stumbled on that article after I was already pursuing that path.

Anyway, it would be nice if ur work with EWTP also resulted in dynamic css chunks (+ HMR). For me pursuing that path stopped when they didn't want HMR a few months back, and didn't seem to care about dynamic splitting. Maybe that's changed??

The latest version here no longer creates 2 js chunks, so the code is basically identical to what you have, with the exception that it has HMR via injecting css file names. What's the latest on what you've been up to? Do you see this stuff as priority for EWTP?

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

No branches or pull requests

2 participants