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

[kbn/optimizer] disable parallelization in terser plugin #58396

Merged
merged 2 commits into from
Feb 25, 2020

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Feb 24, 2020

In order to avoid a memory issue caused when running the build in smaller VMs we should prevent terser from launching worker processes. We're already running in workers and node child processes reserve the same amount of memory as the parent according to https://togithub.com/nodejs/node/issues/25382, additionally, when running on smaller computers we run the optimizer with two workers and 4GB of max-old-space-size for each worker. This combination seems to be exploding on build machines creating builds of the whole stack. Disabling the parallel option in TerserPlugin seems to fix this by running terser in the same process as the webpack compiler so it respects the worker count.

Additionally, rather than limiting the worker number to cpuCount/3, when building the distributable we'll use cpuCount-1 workers because, by default, we only build the dist when it's the only thing happening and we don't have to be quite as considerate to other processes running on the machine (like we try to do in dev).

@tylersmalley tylersmalley changed the title [kbn/optimizer] disable parallelization in terer plugin [kbn/optimizer] disable parallelization in terser plugin Feb 24, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@spalger spalger added release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.7.0 v8.0.0 labels Feb 24, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@spalger spalger marked this pull request as ready for review February 24, 2020 23:19
@spalger spalger requested a review from a team as a code owner February 24, 2020 23:19
@tylersmalley
Copy link
Contributor

tylersmalley commented Feb 25, 2020

While I do see a slight reduction (~7%), I am doubting that this is the cause of the OOM in the release manager given how little memory it's consuming. Still, it's worth the reduction in memory usage and workers. I am going to run some tests during the actual build to see what we're seeing resource wise there.


Wrote a crude bash script to measure the change of memory usage over time:

#!/bin/bash

mem=`free -m | awk 'NR==2{printf "%s", $3}'`

while true; do
        let current_mem=`free -m | awk 'NR==2{printf "%s", $3}'`;
        let mem_diff=current_mem-mem;
        echo $mem_diff;

        sleep 1;
done

Built the platform plugins with the following command while monitoring the usage:

node scripts/build_kibana_platform_plugins.js --no-cache --workers=2

master:

Duration of ~69.07 (69.3, 68.1, 69.8)
Memory usage of ~3,533MB (3332, 3488, 3779)

fix/memory-usage-when-building-dist:

Duration of ~68.57s (68.3, 68, 69.4)
Memory usage of ~3,277MB (3308, 3153, 3370)

@spalger spalger merged commit 3333413 into elastic:master Feb 25, 2020
@spalger spalger deleted the fix/memory-usage-when-building-dist branch February 25, 2020 00:07
@spalger
Copy link
Contributor Author

spalger commented Feb 25, 2020

I am doubting that this is the cause of the OOM in the release manager given how little memory it's consuming

I'm not sure the exact memory usage is relevant, I think the issue is when memory usage is high and then we try to spawn a process it refuses to spawn the process unless there is enough uncommitted ram for the entire process to be doubled... Or something like that...

https://togithub.com/nodejs/node/issues/25382#issuecomment-580319938 is really interesting, this might be even worse once we're on node 12...

spalger pushed a commit to spalger/kibana that referenced this pull request Feb 25, 2020
* [kbn/optimizer] disable parallelization in terer plugin

* use more workers when building the dist
spalger pushed a commit that referenced this pull request Feb 25, 2020
…8421)

* [kbn/optimizer] disable parallelization in terer plugin

* use more workers when building the dist

Co-authored-by: Elastic Machine <[email protected]>
@spalger
Copy link
Contributor Author

spalger commented Feb 25, 2020

7.7/7.x: ce10569

jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 25, 2020
…re/files-and-filetree

* 'master' of github.com:elastic/kibana: (174 commits)
  [SIEM] Fix unnecessary re-renders on the Overview page (elastic#56587)
  Don't mutate error message (elastic#58452)
  Fix service map popover transaction duration (elastic#58422)
  [ML] Adding filebeat config to file dataviz (elastic#58152)
  [Uptime] Improve refresh handling when generating test data (elastic#58285)
  [Logs / Metrics UI] Remove path prefix from ViewSourceConfigur… (elastic#58238)
  [ML] Functional tests - adjust classification model memory (elastic#58445)
  [ML] Use event.timezone instead of beat.timezone in file upload (elastic#58447)
  [Logs UI] Unskip and stabilitize log column configuration tests (elastic#58392)
  [Telemetry] Separate the license retrieval from the stats in the usage collectors (elastic#57332)
  hide welcome screen for cloud (elastic#58371)
  Move src/legacy/ui/public/notify/app_redirect to kibana_legacy (elastic#58127)
  [ML] Functional tests - stabilize typing during df analytics creation (elastic#58227)
  fix short url in spaces (elastic#58313)
  [SIEM] Upgrades cypress to version 4.0.2 (elastic#58400)
  [Index management] Move to new platform "plugins" folder (elastic#58109)
  [kbn/optimizer] disable parallelization in terser plugin (elastic#58396)
  [Uptime] Delete useless try...catch blocks (elastic#58263)
  [Uptime] Use scripted metric for snapshot calculation (elastic#58247) (elastic#58389)
  [APM] Stabilize agent configuration API (elastic#57767)
  ...

# Conflicts:
#	src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants