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

test: log before and after RSS in memory leak test #21080

Closed
wants to merge 2 commits into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jun 1, 2018

Refs: #21076

To help investigate said bug.

CI: https://ci.nodejs.org/job/node-test-pull-request/15212/

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jun 1, 2018
@tniessen
Copy link
Member

tniessen commented Jun 1, 2018

Suggesting to fast-track this.

@tniessen tniessen added the fast-track PRs that do not need to wait for 48 hours to land. label Jun 1, 2018
@Trott
Copy link
Member

Trott commented Jun 1, 2018

+1 to fast-track

@apapirovski
Copy link
Member

@Trott can I take this as you approving fast tracking?

@Trott
Copy link
Member

Trott commented Jun 1, 2018

Re-run Linux CI because debugging another issue left around some root-owned files causing a build to fail: https://ci.nodejs.org/job/node-test-commit-linux/19255/

@Trott
Copy link
Member

Trott commented Jun 1, 2018

@Trott can I take this as you approving fast tracking?

Yes.

@bnoordhuis
Copy link
Member Author

https://ci.nodejs.org/job/node-stress-single-test/1893/ once it starts (predicted: 2 hours from now.)

@nodejs nodejs deleted a comment from refack Jun 1, 2018
@Trott
Copy link
Member

Trott commented Jun 1, 2018

Linux re-run is green.
Stress test is green.
Only thing remaining is to kick off AIX again: https://ci.nodejs.org/job/node-test-commit-aix/15458/

AIX might take a while to start because I'm currently using one (and possibly the only right now) AIX hosts for a stress test. But it will start!

@Trott
Copy link
Member

Trott commented Jun 1, 2018

(By the way, I think @tniessen already got results from this in a CI run off a branch. So the delay in landing this hasn't affected it's usefulness.)

@tniessen
Copy link
Member

tniessen commented Jun 1, 2018

This is the output from https://ci.nodejs.org/job/node-test-commit-linux-containered/4862/nodes=ubuntu1604_sharedlibs_debug_x64/console, its like 5.097MB, so just above the limit. My suggestion in the referenced issue should solve the problem, I can open a PR tomorrow. (Even though I am not sure whether the problem reproduces with the recent v8 update.)

15:20:45 not ok 355 parallel/test-crypto-dh-leak
15:20:45   ---
15:20:45   duration_ms: 2.231
15:20:45   severity: fail
15:20:45   exitcode: 1
15:20:45   stack: |-
15:20:45     assert.js:80
15:20:45       throw new AssertionError(obj);
15:20:45       ^
15:20:45     
15:20:45     AssertionError [ERR_ASSERTION]: before=48488448 after=53833728
15:20:45         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/test/parallel/test-crypto-dh-leak.js:26:8)
15:20:45         at Module._compile (internal/modules/cjs/loader.js:702:30)
15:20:45         at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10)
15:20:45         at Module.load (internal/modules/cjs/loader.js:612:32)
15:20:45         at tryModuleLoad (internal/modules/cjs/loader.js:551:12)
15:20:45         at Function.Module._load (internal/modules/cjs/loader.js:543:3)
15:20:45         at Function.Module.runMain (internal/modules/cjs/loader.js:744:10)
15:20:45         at startup (internal/bootstrap/node.js:261:19)
15:20:45         at bootstrapNodeJSCore (internal/bootstrap/node.js:595:3)
15:20:45   ...

It transpires that the extra bookkeeping in debug builds sometimes makes
the increase in RSS go _just_ over the 5 MB limit, by fewer than 100 kB.
Double the limit so we hopefully don't run into it any time again soon.

The memory leak it tests for was one where RSS grew by hundreds of
megabytes over the lifetime of the test; 5 vs. 10 MB is insignificant.

Fixes: nodejs#21076
@bnoordhuis
Copy link
Member Author

I added another commit that doubles the slop limit, PTAL.

https://ci.nodejs.org/job/node-test-pull-request/15221/
https://ci.nodejs.org/job/node-stress-single-test/1896/ (AIX)
https://ci.nodejs.org/job/node-stress-single-test/1897/ (Linux debug, pending at the time of writing)

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

If I remember correctly, you originally wrote the regression test, so I'll trust your judgement on this one, but in my patch I also increased the iteration count: assuming there is a memory leak, it might go undetected in release builds if the amount of memory leaked per iteration is sufficiently small (which might not be the case, I didn't go into depth in the original issue). The patch which I showed to @Trott yesterday used 5e5 iterations and a limit of 8MB, which i assumed were reasonable choices.

@bnoordhuis
Copy link
Member Author

Forgetting to free a DH key's memory leaks kilobytes at a time so 50k iterations should be plenty to flush that out.

I picked 50k because otherwise it gets really slow with FIPS builds, because you can't use keys < 1024 bits.

@tniessen
Copy link
Member

tniessen commented Jun 2, 2018

Okay, thanks for the explanation, I wasn't sure which part was being leaked. In that case this sounds reasonable.

Trott pushed a commit to Trott/io.js that referenced this pull request Jun 3, 2018
Refs: nodejs#21076

PR-URL: nodejs#21080
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 3, 2018
It transpires that the extra bookkeeping in debug builds sometimes makes
the increase in RSS go _just_ over the 5 MB limit, by fewer than 100 kB.
Double the limit so we hopefully don't run into it any time again soon.

The memory leak it tests for was one where RSS grew by hundreds of
megabytes over the lifetime of the test; 5 vs. 10 MB is insignificant.

Fixes: nodejs#21076

PR-URL: nodejs#21080
Refs: nodejs#21076
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott
Copy link
Member

Trott commented Jun 3, 2018

Landed in 7b6c428...997e97d

@Trott Trott closed this Jun 3, 2018
MylesBorins pushed a commit that referenced this pull request Jun 6, 2018
Refs: #21076

PR-URL: #21080
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 6, 2018
It transpires that the extra bookkeeping in debug builds sometimes makes
the increase in RSS go _just_ over the 5 MB limit, by fewer than 100 kB.
Double the limit so we hopefully don't run into it any time again soon.

The memory leak it tests for was one where RSS grew by hundreds of
megabytes over the lifetime of the test; 5 vs. 10 MB is insignificant.

Fixes: #21076

PR-URL: #21080
Refs: #21076
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants