-
Notifications
You must be signed in to change notification settings - Fork 20
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
Limit the parallelism of user Chromium builds to 8 using an alias #196
base: master
Are you sure you want to change the base?
Conversation
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.
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?
chromium/chromium.dockerfile
Outdated
@@ -4,7 +4,10 @@ FROM janitortechnology/ubuntu-dev | |||
RUN git clone https://chromium.googlesource.com/chromium/tools/depot_tools.git | |||
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 \ |
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.
Nit: Please don't add this unnecessary final \
.
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.
Why?
Kidding, that was a leftover of my first try (adding my line as an && here). Removing, of course.
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 \ |
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.
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.
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.
But then the image building will be slower, or is that not a concern?
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.
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.
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. |
@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. |
I guess I forgot to mention I added the requested comment? |
But temporarily remove the alias before building the image itself.