Create vee-eight-5.0 branch (take 2) by ofrobots · Pull Request #5945 · nodejs/node

ofrobots · 2016-03-29T12:37:08Z

Pull Request check-list

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to [CONTRIBUTING.md][0]?

Affected core subsystem(s)

deps, test

Description of change

Continuation (takeover) of #5592 as @targos isn't available in the next couple of weeks.

Updated to the latest 5.0-lkgr.

R=@bnoordhuis, @nodejs/v8
CI: https://ci.nodejs.org/job/node-test-pull-request/2087/

ofrobots · 2016-03-29T13:12:17Z

test-stringbytes-external-exceed-max-by-2.js and test-stringbytes-external-exceed-max-by-1-binary.js fail on arm pi2. They passed on the previous CI. I wouldn't have expected these to be flaky.

bnoordhuis · 2016-03-29T14:42:35Z

LGTM. No real suggestions for debugging the tests, I'm afraid. Perhaps check whether --gc_interval=1 makes a difference?

indutny · 2016-03-29T14:45:06Z

Rubber-stamp LGTM

orangemocha · 2016-03-31T13:47:08Z

cc @nodejs/node-chakracore

ofrobots · 2016-04-01T19:12:36Z

I investigated the stringbytes test on the arm machines. It seems that they have indeed become flaky on the pi2 arm machines. With V8 5.0, the memory usage has changed subtly enough that common.enoughTestMem occasionally claims that we have enough memory to test, but an actual malloc done by ExternalStringTwoByte::NewFromCopy occasionally fails to allocate memory. Only the pi2 arm machines seem to be at this memory 'sweet spot' (or is it 'sour spot'?).

/cc @Trott because you have looked at flakiness of these tests in the past. One idea may be to adjust the threshold we use in common.enoughTestMem.

Trott · 2016-04-01T20:43:12Z

One idea may be to adjust the threshold we use in common.enoughTestMem.

That's probably the way to go unless/until someone has a better idea.

ofrobots · 2016-04-03T01:16:57Z

I have an idea to fix the flakiness of these tests, but I will have to move them to test/addons. Are test/addons ever run as part of the CI?

Trott · 2016-04-03T02:34:28Z

@ofrobots addons tests are run in CI on all platforms.

/cc @nodejs/build in case I'm misinformed.

jbergstroem · 2016-04-03T07:47:50Z

@Trott said:

@ofrobots addons tests are run in CI on all platforms.

correct: https://github.com/nodejs/node/blob/master/Makefile#L170..L172

The tests used to rely on precise timing of when a JavaScript object would be garbage collected to ensure that there is enough memory available on the system. Switch the test to use a malloc/free pair instead. Ref: nodejs#5945

ofrobots · 2016-04-04T16:59:47Z

Thanks. Here's a PR to fix the flaky tests: #6039. I will rebase once that lands.

The tests used to rely on precise timing of when a JavaScript object would be garbage collected to ensure that there is enough memory available on the system. Switch the test to use a malloc/free pair instead. Ref: nodejs#5945 PR-URL: nodejs#6039 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com> Reviewed-By: evanlucas - Evan Lucas <evanlucas@me.com> Reviewed-By: Trott - Rich Trott <rtrott@gmail.com>

ofrobots · 2016-04-06T03:10:48Z

CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/2171/

ofrobots · 2016-04-06T04:35:18Z

It seems there is a wayward flaky stringbytes-external test-cases was lurking in the parallel directory instead of sequential directory that #6039 missed. 😭

ofrobots · 2016-04-06T14:03:33Z

Rebased after fixing the flaky test here: #6073. New CI: https://ci.nodejs.org/job/node-test-pull-request/2174/ is green.

* Pick up the current branch head for V8 5.0 [1] * Edit v8 gitignore to allow trace_event copy * Update V8 DEP trace_event as per deps/v8/DEPS [2] [1] v8/v8@a480160 [2] https://chromium.googlesource.com/chromium/src/base/trace_event/common/+/4b09207e447ae5bd34643b4c6321bee7b76d35f9 PR-URL: nodejs#5945 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>

V8 5.0 introduced a small modification for the unexpected end of input error. PR-URL: nodejs#5945 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>

jasnell · 2016-04-07T15:38:18Z

@ofrobots ... as we discussed yesterday at the vm summit, will you be opening a PR to get v5 merged into master?

@nodejs/ctc ... @ofrobots and I spoke a bit yesterday about the possibility of getting v8 beta updates landed into master more regularly before they go stable so long as CI is green. Specifically, I'd like to get the v5 beta into the v6 release candidate that I'll be putting together on Monday.. so it would be fantastic if we can get v5 into master even tho it hasn't gone completely stable yet.

ofrobots · 2016-04-08T00:54:04Z

PR for getting V8 5.0 into master: #6111.

* Pick up the branch head for V8 5.0 stable [1] * Edit v8 gitignore to allow trace_event copy * Update V8 DEP trace_event as per deps/v8/DEPS [2] [1] https://chromium.googlesource.com/v8/v8.git/+/3c67831 [2] https://chromium.googlesource.com/chromium/src/base/trace_event/common/+/4b09207e447ae5bd34643b4c6321bee7b76d35f9 Ref: nodejs#5945 PR-URL: nodejs#6111 Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com> Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>

V8 5.0 introduced a small modification for the unexpected end of input error. PR-URL: nodejs#5945 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>

V8 5.0 introduced a small modification for the unexpected end of input error. PR-URL: #5945 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>

* Pick up the branch head for V8 5.0 stable [1] * Edit v8 gitignore to allow trace_event copy * Update V8 DEP trace_event as per deps/v8/DEPS [2] [1] https://chromium.googlesource.com/v8/v8.git/+/3c67831 [2] https://chromium.googlesource.com/chromium/src/base/trace_event/common/+/4b09207e447ae5bd34643b4c6321bee7b76d35f9 Ref: #5945 PR-URL: #6111 Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com> Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>

V8 5.0 introduced a small modification for the unexpected end of input error. PR-URL: #5945 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>

The tests used to rely on precise timing of when a JavaScript object would be garbage collected to ensure that there is enough memory available on the system. Switch the test to use a malloc/free pair instead. Ref: nodejs#5945 Ref: nodejs#6039 Ref: nodejs#6073

The tests used to rely on precise timing of when a JavaScript object would be garbage collected to ensure that there is enough memory available on the system. Switch the test to use a malloc/free pair instead. Ref: #5945 Ref: #6039 Ref: #6073 PR-URL: #6705 Reviewed-By: James M Snell <jasnell@gmail.com>

ofrobots mentioned this pull request Mar 29, 2016
Create vee-eight-5.0 branch #5592
Closed
4 tasks

ofrobots added the v8 engine Issues and PRs related to the V8 dependency. label Mar 29, 2016

ofrobots mentioned this pull request Apr 4, 2016
test: fix flakiness of stringbytes-external #6039
Closed
3 tasks

ofrobots force-pushed the vee-eight-5.0 branch from 0fa913a to c45a15aCompare April 6, 2016 03:09

ofrobots force-pushed the vee-eight-5.0 branch from c45a15a to 68a745cCompare April 6, 2016 13:35

ofrobotsand others added 2 commits April 6, 2016 07:04

test: update error message for JSON.parse
c01a9cc
V8 5.0 introduced a small modification for the unexpected end of input error. PR-URL: nodejs#5945 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>

ofrobots force-pushed the vee-eight-5.0 branch from 68a745c to c01a9ccCompare April 6, 2016 14:06

ofrobots merged commit c01a9cc into nodejs:vee-eight-5.0Apr 6, 2016

ofrobots deleted the vee-eight-5.0 branch April 6, 2016 14:10

ofrobots mentioned this pull request Apr 8, 2016
Upgrade to V8 5.0.71.32 in master #6111
Merged
2 tasks

MylesBorins added dont-land-on-v5.x labels Apr 20, 2016

Uh oh!

Uh oh!

Conversation

ofrobots commented Mar 29, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

ofrobots commented Mar 29, 2016

Uh oh!

bnoordhuis commented Mar 29, 2016

Uh oh!

indutny commented Mar 29, 2016

Uh oh!

orangemocha commented Mar 31, 2016

Uh oh!

ofrobots commented Apr 1, 2016

Uh oh!

Trott commented Apr 1, 2016

Uh oh!

ofrobots commented Apr 3, 2016

Uh oh!

Trott commented Apr 3, 2016

Uh oh!

jbergstroem commented Apr 3, 2016

Uh oh!

ofrobots commented Apr 4, 2016

Uh oh!

ofrobots commented Apr 6, 2016

Uh oh!

ofrobots commented Apr 6, 2016

Uh oh!

ofrobots commented Apr 6, 2016

Uh oh!

jasnell commented Apr 7, 2016

Uh oh!

ofrobots commented Apr 8, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants