-
-
Notifications
You must be signed in to change notification settings - Fork 180
fix(uglify/index): don't use a worker farm unless more than one process is available #280
fix(uglify/index): don't use a worker farm unless more than one process is available #280
Conversation
@hedgepigdaniel Looks like it is save memory only for one core system (one thread), right? |
The difference is for a two core system - since the default number of workers is Tbh, I haven't tested that is uses less memory but I assume that there is at least some memory that is shared in one process that needs to be duplicated in a second one. I tested that it works though 👍 |
@hedgepigdaniel Looks you are right today, i am testing memory usage and i think we can merge this 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you additionally try with os.cpus().length
instead of os.cpus().length - 1
and see if this works ?
@michael-ciniawsky
|
The |
@michael-ciniawsky it is good practice, If at least one thread completed it is require |
@michael-ciniawsky Also using |
@evilebottnawi We don't certainly know for sure if this would be the case or not, I'm not disagreeing but by at least trying we might get proof of the circumstances here |
@michael-ciniawsky why we should create new thread on one core system? It is does not have any sense |
Each creating new thread using more memory and cpu |
@michael-ciniawsky You can justify in detail what you do not agree with PR, or approve PR |
I don't disagree with anything in this PR, but maybe using |
@michael-ciniawsky What exactly do you disagree? |
In short:
|
Agree with @evilebottnawi here. If you don't have one master thread, you end up context switching all over the place and take a huge performance hit and may as well not thread it at all. Reserving one thread to orchestrate execution has been a generally accepted practice pretty much since we started leveraging the power of multi-core processors. |
@d3viant0ne can we merge this? i don't have administration right here |
This should save some memory due to sharing common data within one process instead of spawning a second one.