Skip to content

Limit the parallelism of user Chromium builds to 8 using an alias#196

Open
phistuck wants to merge 6 commits into
JanitorTechnology:masterfrom
phistuck:parallelism-limiter
Open

Limit the parallelism of user Chromium builds to 8 using an alias#196
phistuck wants to merge 6 commits into
JanitorTechnology:masterfrom
phistuck:parallelism-limiter

Conversation

@phistuck

Copy link
Copy Markdown

But temporarily remove the alias before building the image itself.

@nt1m nt1m requested a review from jankeromnes May 21, 2018 17:37

@jankeromnes jankeromnes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for fixing our recurrent OOM problem!

Assuming that ~/.bash_aliases is actually sourced, this might work, but I don't know if using a ninja alias is actually the best solution.

@ishitatsuyuki you mentioned some affinity host-level setting? Also I feel like you would be the best person to approve or reject this pull request, may I defer this review to you?

Comment thread chromium/chromium.dockerfile Outdated
ENV PATH $PATH:/home/user/depot_tools
RUN echo "\n# Add Chromium's depot_tools to the PATH." >> .bashrc \
&& echo "export PATH=\"\$PATH:/home/user/depot_tools\"" >> .bashrc
&& echo "export PATH=\"\$PATH:/home/user/depot_tools\"" >> .bashrc \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Please don't add this unnecessary final \.

@phistuck phistuck May 22, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Kidding, that was a leftover of my first try (adding my line as an && here). Removing, of course.

Comment thread chromium/chromium-update.dockerfile Outdated
RUN cd /home/user/chromium/src \
# Remove the parallelism limited Ninja alias and
# update and rebuild Chromium's source code.
RUN unalias ninja \

@jankeromnes jankeromnes May 22, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'm not sure it's necessary to have two different behaviors (normal developers use half of cores vs update Dockerfile uses all cores), so I don't think you need to unalias here. Also maybe we should remove the -j`nproc` ninja parameter below.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then the image building will be slower, or is that not a concern?

@jankeromnes jankeromnes May 22, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we import *-update.dockerfile into our admin interface, for when we want to update some project image by just building a few layers on top of it instead of pulling a brand new image (we stopped using *-update.dockerfile for most projects, but we still do it for Chrome, because we can't pre-build the Chrome browser in the main Dockerfile as it makes CircleCI time out, so we use the chromium-update.dockerfile to pre-build Chrome on-premises in OVH1).

And since the -j`nproc` was OOM'ing OVH1, I manually edited it to -j8 in our admin interface... So TL;DR we should only have one way to build Chrome, to be used everywhere by developers and CI alike.

@phistuck

Copy link
Copy Markdown
Author

If I understand the concept correctly, I feel like host-level affinity limits would be too limiting for the situations where someone needs a bit more power for a short while and the load is very low, but maybe I am just paranoid.

@ishitatsuyuki

Copy link
Copy Markdown
Collaborator

@phistuck The affinity limits is considered to affect the performance very minimally: I only plan to shut down the hyperthreads, which at most affects ~15% performance.

This looks fine though. Let's add a note so we don't forget to revert this once we have affinity implementation.

@phistuck

phistuck commented Aug 4, 2018

Copy link
Copy Markdown
Author

I guess I forgot to mention I added the requested comment?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants