test: speed up stringbytes-external test #3005
Conversation
|
LGTM |
1 similar comment
|
LGTM |
|
CI: https://ci.nodejs.org/job/node-test-pull-request/363/ EDIT: immediate scroll from top to bottom omitted showing the first CI in my browser window. |
| @@ -112,9 +112,11 @@ var PRE_3OF4_APEX = Math.ceil((EXTERN_APEX / 4) * 3) - RADIOS; | |||
| (function() { | |||
| // v8::String::kMaxLength defined in v8.h | |||
| const kStringMaxLength = process.binding('buffer').kStringMaxLength; | |||
| const buf1 = new Buffer(kStringMaxLength + 1); | |||
| const buf2 = new Buffer(kStringMaxLength); | |||
There was a problem hiding this comment.
These allocations need to be after the try catch else it would fail on Raspberry Pi 1/2.
There was a problem hiding this comment.
awesome save. Thanks for catching this.
|
Weird...it seems like the test is slower on all of the CI machines with this? Locally, it is about a 20% improvement... |
evanlucas
force-pushed
the
speedupesb
branch
from
121f165 to
0479dcb
Compare
| @@ -126,12 +122,19 @@ var PRE_3OF4_APEX = Math.ceil((EXTERN_APEX / 4) * 3) - RADIOS; | |||
| return; | |||
| } | |||
|
|
|||
| const buf1 = new Buffer(kStringMaxLength + 1); | |||
| const buf2 = new Buffer(kStringMaxLength); | |||
There was a problem hiding this comment.
Probably, buf2 can be moved below and created only when it is actually used.
evanlucas
force-pushed
the
speedupesb
branch
2 times, most recently
from
47d5f36 to
ebc1aef
Compare
|
Made some changes with the help of @trevnorris. Should be improved quite a bit more now. New CI: https://ci.nodejs.org/job/node-test-pull-request/368/ |
| @@ -126,50 +122,57 @@ var PRE_3OF4_APEX = Math.ceil((EXTERN_APEX / 4) * 3) - RADIOS; | |||
| return; | |||
| } | |||
|
|
|||
| const buf0 = new Buffer(kStringMaxLength * 2 + 2); | |||
There was a problem hiding this comment.
Can't we slice this off the Buffer allocated in the try..catch above?
There was a problem hiding this comment.
That would work. my worry is that we allocate 256MB more than needed above, and if we hang on to that then the tests that turn these into strings will need to increase the heap by an additional 256MB. Hence why over allocating in the check above is a small way to ensure the heap has enough space for the toString() calls below.
|
This is still haunting the CI, LGTM. The improvement looks to be around 20% right now, still worth it imho: |
|
@mscdex and @trevnorris mind taking a look again since changes have been made since your signed off on it? |
|
Changes LGTM |
|
Maybe this will need to be broken into two tests so as to not exceed the time limit. |
|
You want me to go ahead and do that? |
|
I think I'd also prefer a split, but that can be done in another PR. |
|
@evanlucas Don't worry about splitting it now. We can handle that later. If CI on latest changes is happy then I'd say land it. (on vacation ATM so can't do it myself) |
|
Still failing on arm. I've got a Pi2 I'm messing with locally, so will try to split the test up and see if that helps |
|
@evanlucas Still wouldn't worry about the failing tests on arm. They were failing before. This PR is an incremental improvement, and splitting the test up can happen in another PR. I move we land this as is. |
test-stringbytes-external tends to take quite a while on slower hardware. A lot of the time is taken by creating a new buffer that is very large. The improvements come from reusing the same buffer. PR-URL: nodejs#3005 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
|
Landed in 7d66749. Thanks! |
test-stringbytes-external tends to take quite a while on slower hardware. A lot of the time is taken by creating a new buffer that is very large. The improvements come from reusing the same buffer. PR-URL: #3005 Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>
ChALkeR
added
the
performance
test-stringbytes-external tends to take quite a while on slower
hardware. A lot of the time is taken by creating a new buffer that is
very large. The improvements come from reusing the same buffer.
Before:
After:
Related: #2370
R= @trevnorris