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

fix(@angular-devkit/build-angular): limit the amount of CPUs used by workers #17028

Merged
merged 1 commit into from Feb 25, 2020
Merged

fix(@angular-devkit/build-angular): limit the amount of CPUs used by workers #17028

merged 1 commit into from Feb 25, 2020

Conversation

alan-agius4
Copy link
Collaborator

@alan-agius4 alan-agius4 added the target: patch This PR is targeted for the next patch release label Feb 20, 2020
@Zwartpet
Copy link

Would really appreciate it if the workersCount would be configurable. I would like to opt-out on the parallel workers

@filipesilva
Copy link
Contributor

@Zwartpet we follow the principle that less options is generally better than more options, and if we can do the "right thing" we should just do that. This link shows good reasons for avoiding more options when possible.

I understand you might not agree with it, or it might not cover your intended use case, but it is a principle that seems to work well for us so far so we'd be reticent to go against it unless other options are exhausted.

@Zwartpet
Copy link

@filipesilva
I understand that less options make it better. But because there is no way to determine the actual cpu count available in a container, it seems to me that there is no "right thing".

@filipesilva
Copy link
Contributor

Is that accurate though? I mean, I understand the exact count might not be available. But that doesn't mean there's no "right thing". Maybe the right thing is to disable parallelism when we can't determine the exact count. Or maybe to be conservative and use a number that sounds good.

My original changes in #16978 were aimed at being conservative: if it looks like there are tons of cpus maybe just use 8 total. I don't know if that covers all cases but it sounded like it was a good attempt. It still sounds like a good attempt, with the caveat that it didn't cover all uses of parallel workers, which this PR tries to address.

Do you think that even with these changes your project will fail to build in the container you are using?

@Zwartpet
Copy link

Zwartpet commented Feb 20, 2020

I'm pretty sure it will still fail. I'm running a pipeline in k8s with multiple concurrent build tasks (eg 4).
All of the angular builds will see the number of cpu's available on the host vm (eg: 8) and will spawn this many workers.

That means that 32 workers will be created on a vm with only 8 cpu's. And those cpu's might not even be available as there might be other jobs that run in the same k8s cluster (so on the same host vm).

I could probably solve my issues by upgrading my k8s cluster. But this setup has worked through all the angular versions since angular-2. So I don't really see upgrading the k8s cluster as 'the' solution.

Edit
To clarify: the exact count is available but it measures the count on the host machine, and not the count that was made available to the container

@filipesilva
Copy link
Contributor

Ah so your usecase is that you have multiple concurrent build tasks... sorry, I hadn't copped on to that yet. Interesting that it worked before though. We've been doing the parallel builds since.... as far as I remember really. At least since CLI 6, but I'm pretty sure we did parallel ever since we started using webpack. I guess maybe the builds lucked out and were never doing the parallel bits at the same time before?

@clydin
Copy link
Member

clydin commented Feb 20, 2020

An important element in this situation is that all parallelism is not equal. Due to Node.js's default memory limit of ~2GB, thread-based workers are not viable for memory intensive workloads. As a result, process-based workers are used for tasks with higher memory requirements. The downside of this, in this context, is that process-based node workers use more memory since they are a full, separate instance of a Node.js application.

While having 32 workers on 8 physical CPUs may not be ideal in this situation, it is not in itself a problem. The OS manages 100s of processes normally. The issue here is one of memory usage and is due to the interaction of several different items. The first is that the Node.js API returns virtual CPUs and in containers this can provide inaccurate and exaggerated numbers. The second is that within the CLI that number is used to start a process-based worker pool. Even an idle worker could use 50-100MB. If the container that is reporting a large amount of CPUs also does not have the necessary memory to support the pool, then an OOM crash will result.

@Zwartpet
Copy link

What's the best way to navigate this issue then? Simply adding more memory will probably work for a while, but might not work for everybody.

@clydin
Copy link
Member

clydin commented Feb 20, 2020

I think this PR is a good first step. It should mitigate the issue for most users. The next step would be to investigate improved heuristics for the worker pool sizes. Accounting for a machine's available memory and a median per worker budget may be a promising avenue to pursue. This should ideally remove the need for a separate option. However, for edge cases, the potential for an option or environment variable (since the issue is more machine/environment based than project based) will be discussed with the team as well.

@dgp1130
Copy link
Collaborator

dgp1130 commented Feb 24, 2020

@alan-agius4, looks like this has a merge conflict with master. I can merge it once that gets taken care of.

@dgp1130 dgp1130 removed the action: merge The PR is ready for merge by the caretaker label Feb 24, 2020
@alan-agius4 alan-agius4 added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Feb 24, 2020
@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Feb 25, 2020
@angular angular deleted a comment from ngbot bot Feb 25, 2020
@dgp1130 dgp1130 merged commit 94c753c into angular:master Feb 25, 2020
@dgp1130
Copy link
Collaborator

dgp1130 commented Feb 25, 2020

Also merged to 9.0.x patch branch.

@alan-agius4 alan-agius4 deleted the cpu-core-count branch February 25, 2020 18:44
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants