test: reduce memory use in test stringbytes #3697

mcornac · 2015-11-06T19:29:54Z

When I run the string bytes tests dealing with strings of length kStringMaxLength or more on a Windows test machine with 4GB memory I see the tests skipped or failed with the following outputs respectively.

1..0 # Skipped: intensive toString tests due to memory confinements
RangeError: Invalid array buffer length

The code being used to determine if the test should be skipped is:

 try {
  new Buffer(kStringMaxLength * 3);
 } catch(e) {
  assert.equal(e.message, 'Invalid array buffer length');
  console.log(
       '1..0 # Skipped: intensive toString tests due to memory confinements');
   return;
 }

A problem with this approach is that the garbage collector does not necessarily run between the tried allocation and the necessary allocations later. That means that even though no exception is thrown in the try block since the machine has enough memory, the later allocations fail since the memory is now taken.

To verify this, I ran the test with Release\node.exe --expose-gc test\parallel\test-stringbytes-external-at-max.js to expose global.gc() which I used after the try catch block. With this setup the test would either skip or succeed as desired.

To solve the issue, I removed the Buffer allocations that were used to check if the tests should be skipped and moved the actual Buffer allocations into the try block so the test can still be skipped if memory allocation fails here. The tests should now use less total memory and be less likely to be skipped and more importantly, less likely to fail due to a memory constraint when they are not skipped.

targos · 2015-11-06T19:49:13Z

The fix looks good, but why did you change the assertions in the catch blocks?

mcornac · 2015-11-06T20:09:51Z

I'm not sure if there are disadvantages but I thought it could be helpful to rethrow the original exception in case a different exception occurs.
E.g.

/sandbox/cornacch/node/test/parallel/test-stringbytes-external-at-max.js:15
  if (e.message != 'Invalid array buffer length') throw (e);
                                                  ^
Error: toString failed
    at Buffer.toString (buffer.js:382:11)
    at Object.<anonymous> (/sandbox/cornacch/node/test/parallel/test-stringbytes-external-at-max.js:13:17)

Instead of

assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: 'toString failed' == 'Invalid array buffer length'
    at Object.<anonymous> (/sandbox/cornacch/node/test/parallel/test-stringbytes-external-at-max.js:15:10)

mhdawson · 2015-11-06T20:55:32Z

lgtm, I do think re-throwing makes sense so that we can see the original exception

trevnorris · 2015-11-06T23:30:39Z

The reason behind the current implementation is that it helps account for the additional space in the v8 heap necessary to store all the strings that are created. I'd say the most reliable approach would be to run gc right after the try/catch.

mcornac · 2015-11-09T21:43:50Z

@trevnorris I made some changes to the at-max test to use --expose-gc based on some other tests. Can you please take a look before I update the other tests?

trevnorris · 2015-11-09T22:10:27Z

@thinkingdust Nice. Like the way you did that.

mcornac · 2015-11-09T23:39:22Z

Thanks. I've pushed the changes to the other files. I also ran the tests on the Windows machine again and they always either skip or pass now as desired.

trevnorris · 2015-11-10T16:43:39Z

I'd squash the commits and include a note about why the change to throwing instead of assert.

Other than that, LGTM.

bnoordhuis · 2015-11-10T18:37:23Z

test/parallel/test-stringbytes-external-at-max.js

 } catch(e) {
-  assert.equal(e.message, 'Invalid array buffer length');
+  if (e.message != 'Invalid array buffer length') throw (e);


I'd add a comment explaining you don't use assert in order to preserve the original exception.

Oh, and a femto style nit: !==

bnoordhuis · 2015-11-10T18:43:16Z

Some style nits but basic premise LGTM.

mcornac · 2015-11-10T21:35:10Z

@bnoordhuis Thanks for the suggestions, updated.

bnoordhuis · 2015-11-10T21:41:10Z

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

jasnell · 2015-11-12T19:49:45Z

Kicked off a new CI run... the previous run (693) was returning a 404: https://ci.nodejs.org/job/node-test-pull-request/712/

targos · 2015-11-12T20:02:21Z

LGTM

mcornac · 2015-11-12T22:11:55Z

I think there is only one related failure.
test-stringbytes-external-exceed-max-by-1-base64.js failed with a timeout here:
https://ci.nodejs.org/job/node-test-binary-arm/469/RUN_SUBSET=1,nodes=pi1-raspbian-wheezy/console
but the same test passed here:
https://ci.nodejs.org/job/node-test-binary-arm/469/RUN_SUBSET=1,nodes=pi2-raspbian-wheezy/console

Fishrock123 · 2015-11-13T15:41:12Z

Maybe just exclude this test on low memory machines? Something like

if (os.totalmem() <= 512000000 /* 512mb */) {
  // bail
  return;
}

(note: totalmem() returns bytes)

Edit actually maybe freemem() would be better? Idk.

mhdawson · 2015-11-16T21:37:08Z

So if I understand correctly @thinkingdust we still saw a failure in the test even after the PR ?

mcornac · 2015-11-16T22:28:17Z

It's a 404 now but there was a timeout failure in one of the tests on Raspberry Pi.
I don't know how v8 decides if a Buffer allocation is valid. The test needs about 756MB (3 * kStringMaxLength) while I'm guessing the Pi has 512MB but it seems like the test was not skipped but instead ran very slowly.

trevnorris · 2015-11-16T23:06:16Z

@thinkingdust v8 doesn't have anything to do with memory allocation with Buffers. It's a simple malloc() call, and v8 uses whatever the system returns. In these cases I believe that the allocation is coming from swap space, hence why operations on the memory returned is incredibly slow. I think @Fishrock123's idea is worth trying out.

mcornac · 2015-11-17T05:52:46Z

I added a freemem check for the at-max test and it works fine on my Linux machine. I can try it on the Windows machine tomorrow. Is this use of freemem reliable enough to remove the try catch blocks?

mcornac · 2015-11-17T16:28:21Z

Using the freemem check works to always skip or pass on the original Windows machine.

mcornac · 2015-11-17T16:49:41Z

@trevnorris I changed the at-max test to check using freemem instead of try/catch and gc. If that looks okay to you I'll update the other files too.

mhdawson · 2015-11-20T16:39:48Z

Another failure on ARM today: https://ci.nodejs.org/job/node-test-binary-arm/81/RUN_SUBSET=4,nodes=pi2-raspbian-wheezy/console

Would be nice to get the fix in.

jasnell · 2015-11-20T17:33:31Z

@trevnorris ... can you take another look at this when you get a moment? :-)

Fishrock123 · 2015-11-20T18:56:48Z

Some things: I think freemem may include cache and possibly swap, but I can confirm totalmem is just the hardware ram.

I think my original suggestion of just skipping on 512mb ram machines (ontop of the other protections) would be best.

trevnorris · 2015-11-30T21:25:28Z

Had a follow up to @Fishrock123's comment, but still LGTM.

mcornac · 2015-12-01T16:35:49Z

@trevnorris @Fishrock123 I added common.kMaxTestMemUsage. Can you take a look?

jasnell · 2015-12-01T16:55:10Z

LGTM

Fishrock123 · 2015-12-01T17:56:27Z

Correct me if I'm wrong but I think kMaxTestMemUsage needs to be set-able from C++ to be of value?

Fishrock123 · 2015-12-01T17:56:39Z

Or at least an environment variable.

trevnorris · 2015-12-01T18:54:36Z

@Fishrock123 Don't think so. We've honestly just selected the value based on past fail cases. Can't see why it would need to be configurable and/or need to be set from C++.

Fishrock123 · 2015-12-01T21:31:15Z

Ah, ok. I think exports.enoughTestMem = os.totalmem() > 0x20000000; might be better, but maybe don't hold the PR on it.

trevnorris · 2015-12-01T23:06:00Z

So make the export simply a boolean? Can't say I'm against that, but also don't want to make ~~@mhdawson~~ @thinkingdust rage quit this PR. ;-)

EDIT: auto-select fail

mcornac · 2015-12-02T16:09:40Z

@Fishrock123 @trevnorris

Fishrock123 · 2015-12-02T16:17:06Z

Rubber-stamp LGTM

mcornac · 2015-12-02T16:40:41Z

🎆 ✨

trevnorris · 2015-12-02T19:32:06Z

LGTM

trevnorris · 2015-12-02T19:34:30Z

@thinkingdust i'm having a hard time segmenting these commits into discrete changes. Mind if I squash them all, or would you like to take care of it?

The current implementation of tests for strings with length at or exceeding kStringMaxLength allocate a temporary buffer inside a try block to skip the test if there is insufficient memory. This commit adds an invocation of the garbage collector after the temporary buffer is allocated so that memory is freed for later allocations. Change the corresponding catch block to rethrow the original exception instead of asserting the exception message to provide more information about the exception. Add an additional check before trying to allocate memory to immediately skip the test on machines with insufficient total memory.

mcornac · 2015-12-02T21:58:29Z

I think 1 commit works. Squashed.

trevnorris · 2015-12-02T22:07:45Z

Thanks much! Landed in a40b9ca

The current implementation of tests for strings with length at or exceeding kStringMaxLength allocate a temporary buffer inside a try block to skip the test if there is insufficient memory. This commit adds an invocation of the garbage collector after the temporary buffer is allocated so that memory is freed for later allocations. Change the corresponding catch block to rethrow the original exception instead of asserting the exception message to provide more information about the exception. Add an additional check before trying to allocate memory to immediately skip the test on machines with insufficient total memory. PR-URL: #3697 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

The current implementation of tests for strings with length at or exceeding kStringMaxLength allocate a temporary buffer inside a try block to skip the test if there is insufficient memory. This commit adds an invocation of the garbage collector after the temporary buffer is allocated so that memory is freed for later allocations. Change the corresponding catch block to rethrow the original exception instead of asserting the exception message to provide more information about the exception. Add an additional check before trying to allocate memory to immediately skip the test on machines with insufficient total memory. PR-URL: nodejs#3697 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]>

mscdex added test Issues and PRs related to the tests. buffer Issues and PRs related to the buffer subsystem. memory Issues and PRs related to the memory management or memory footprint. labels Nov 6, 2015

mhdawson added the lts-watch-v4.x label Nov 6, 2015

bnoordhuis reviewed Nov 10, 2015
View reviewed changes

trevnorris closed this Dec 2, 2015

This was referenced Dec 8, 2015

Release proposal: v5.2.0 #4180

Closed

Release proposal: v5.2.0 (Stable) #4181

Merged

rvagg mentioned this pull request Dec 17, 2015

Release 4.2.4 Planning #4321

Closed

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Dec 29, 2015

MylesBorins mentioned this pull request Jan 19, 2016

V4.2.5 proposal #4768

Merged

test: reduce memory use in test stringbytes #3697

test: reduce memory use in test stringbytes #3697

Conversation

mcornac commented Nov 6, 2015

targos commented Nov 6, 2015

mcornac commented Nov 6, 2015

mhdawson commented Nov 6, 2015

trevnorris commented Nov 6, 2015

mcornac commented Nov 9, 2015

trevnorris commented Nov 9, 2015

mcornac commented Nov 9, 2015

trevnorris commented Nov 10, 2015

bnoordhuis Nov 10, 2015

Choose a reason for hiding this comment

bnoordhuis Nov 10, 2015

Choose a reason for hiding this comment

bnoordhuis commented Nov 10, 2015

mcornac commented Nov 10, 2015

bnoordhuis commented Nov 10, 2015

jasnell commented Nov 12, 2015

targos commented Nov 12, 2015

mcornac commented Nov 12, 2015

Fishrock123 commented Nov 13, 2015

mhdawson commented Nov 16, 2015

mcornac commented Nov 16, 2015

trevnorris commented Nov 16, 2015

mcornac commented Nov 17, 2015

mcornac commented Nov 17, 2015

mcornac commented Nov 17, 2015

mhdawson commented Nov 20, 2015

jasnell commented Nov 20, 2015

Fishrock123 commented Nov 20, 2015

trevnorris commented Nov 30, 2015

mcornac commented Dec 1, 2015

jasnell commented Dec 1, 2015

Fishrock123 commented Dec 1, 2015

Fishrock123 commented Dec 1, 2015

trevnorris commented Dec 1, 2015

Fishrock123 commented Dec 1, 2015

trevnorris commented Dec 1, 2015

mcornac commented Dec 2, 2015

Fishrock123 commented Dec 2, 2015

mcornac commented Dec 2, 2015

trevnorris commented Dec 2, 2015

trevnorris commented Dec 2, 2015

mcornac commented Dec 2, 2015

trevnorris commented Dec 2, 2015