Time to play with V8 4.6 #2688

targos · 2015-09-04T08:55:04Z

Now that 4.5 is on master, we can start working on the vee-eight-4.6 integration branch.
I upgraded V8 to 4.6.85.12 (https://github.com/nodejs/node/tree/vee-eight-4.6). Sadly I cannot make it to compile. Here is the error I get:

node [vee-eight-4.6] % make 
make -C out BUILDTYPE=Release V=1
make[1]: Entering directory '/home/mzasso/git/targos/node/out'
  g++ '-DV8_TARGET_ARCH_X64' '-DENABLE_DISASSEMBLER' '-DV8_IMMINENT_DEPRECATION_WARNINGS' '-DICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_STATIC' -I../deps/v8  -pthread -Wall -Wextra -Wno-unused-parameter -m64 -B/home/mzasso/git/targos/node/third_party/binutils/Linux_x64/Release/bin -fno-strict-aliasing -m64 -O3 -ffunction-sections -fdata-sections -fno-omit-frame-pointer -fdata-sections -ffunction-sections -O3 -fno-rtti -fno-exceptions -std=gnu++0x -MMD -MF /home/mzasso/git/targos/node/out/Release/.deps//home/mzasso/git/targos/node/out/Release/obj.target/v8_base/gen/debug-support.o.d.raw  -c -o /home/mzasso/git/targos/node/out/Release/obj.target/v8_base/gen/debug-support.o /home/mzasso/git/targos/node/out/Release/obj/gen/debug-support.cc
/home/mzasso/git/targos/node/out/Release/obj/gen/debug-support.cc:385:49: error: ‘kInObjectPropertiesOffset’ is not a member of ‘v8::internal::Map’
 int v8dbg_class_Map__inobject_properties__int = Map::kInObjectPropertiesOffset;
                                                 ^
deps/v8/tools/gyp/v8_base.target.mk:420: recipe for target '/home/mzasso/git/targos/node/out/Release/obj.target/v8_base/gen/debug-support.o' failed
make[1]: *** [/home/mzasso/git/targos/node/out/Release/obj.target/v8_base/gen/debug-support.o] Error 1
make[1]: Leaving directory '/home/mzasso/git/targos/node/out'
Makefile:45: recipe for target 'node' failed
make: *** [node] Error 2

I am able to compile d8 without any issue on the same version.

/cc @nodejs/v8

The text was updated successfully, but these errors were encountered:

indutny · 2015-09-04T08:58:41Z

They broke it again! 😢

targos · 2015-09-04T09:00:34Z

Hold on, 4.6.85.13 was just released with a fix from @ofrobots 🎉

targos · 2015-09-04T09:59:08Z

OK it's all working on my side, here is a first CI: https://ci.nodejs.org/job/node-test-commit/501/
I did not apply any floating patch yet, something may be missing.

ofrobots · 2015-09-04T15:12:33Z

@McFarts Here's the V8 team's blog post on 4.6: http://v8project.blogspot.de/2015/08/v8-release-46.html

There are no plans that I am aware of to add shared-memory multi-threading to V8 as JavaScript is single threaded as per the language spec. Perhaps your multi-core use-cases can be addressed by the cluster module, or perhaps by workers (#2133) if/when they end up landing in Node.

Fishrock123 · 2015-09-04T15:13:34Z

When do you think they will enable memory sharing across cores? I need my global variables and data to be... you know.. really global :P If you know what I mean

Knowing your aforementioned use case (game server?) I think you should focus on other optimizations. You can't multi-thread that in JavaScript.

Fishrock123 · 2015-09-04T17:21:52Z

@McFarts Could you please ask this on stack-overflow instead? This really isn't the place for the questions you are asking.

kkoopa · 2015-09-05T17:36:05Z

NAN 2.0.8 test suite passes.

ChALkeR · 2015-09-10T15:30:04Z

Could you please check if #2793 works in 4.6?

ChALkeR · 2015-09-18T18:47:05Z

v8 4.6.85.19 should fix #2793.

targos · 2015-09-25T20:10:06Z

I just rebased vee-eight-4.6 on master and included the backports from 4.7 that I could:

acb6779, 4e028bd, 75fe773 and c22bcd3 applied cleanly.
31450fc and 2b8a06b introduce conflicts. cc @indutny

trevnorris · 2015-09-25T22:38:23Z

This ready for a CI run?

targos · 2015-09-26T05:50:30Z

https://ci.nodejs.org/job/node-test-commit/674/

targos · 2015-09-26T06:03:17Z

... the .gitignore trolled me again: https://github.com/nodejs/node/blob/master/.gitignore#L26

targos · 2015-09-26T06:20:33Z

OK now it's compiling: https://ci.nodejs.org/job/node-test-commit/675/

targos · 2015-09-26T10:15:19Z

Looks promising. There is just one failing test on some platforms:

not ok 687 test-stringbytes-external.js
#assert.js:311
#    throw actual;
#    ^
#
#RangeError: Invalid array buffer length
#    at new ArrayBuffer (native)
#    at new Uint8Array (native)
#    at allocate (buffer.js:98:17)
#    at new Buffer (buffer.js:49:12)
#    at /home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1404-32/test/parallel/test-stringbytes-external.js:138:5
#    at Function._throws (assert.js:293:5)
#    at Function.assert.throws (assert.js:319:11)
#    at /home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1404-32/test/parallel/test-stringbytes-external.js:137:10
#    at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-linux/nodes/ubuntu1404-32/test/parallel/test-stringbytes-external.js:175:3)
#    at Module._compile (module.js:434:26)
  ---
  duration_ms: 1.210

trevnorris · 2015-09-26T19:01:11Z

@targos That implies allocation failed. v8 chose one stupid and cryptic error message for that.

trevnorris · 2015-09-26T19:04:03Z

Though that stack looks like an older Buffer implementation.

targos · 2015-09-26T19:35:35Z

@trevnorris what do you mean ? The line numbers match with what's on master.

trevnorris · 2015-09-26T20:02:00Z

@targos my bad. got implementations mixed up. But as for the test, may need to add extra conditionals in the tests to make sure allocation failure doesn't cause the test to fail. Though that would lead me to believe v8 is consuming more memory? Since it didn't fail before.

skomski · 2015-09-26T22:17:48Z

The problem it seems is that v8 4.6> doesn't initiate a mark-sweep and then simply fails the allocation.
Added some console.log(process.memoryUsage()); between the buffer allocations in the test.

./node_v45 --trace_gc test/parallel/test-stringbytes-external

[20822:0x2524d30]       31 ms: Scavenge 2.1 (38.0) -> 1.8 (38.0) MB, 0.4 / 0 ms [allocation failure].
[20822:0x2524d30]       31 ms: Scavenge 2.1 (38.0) -> 2.1 (39.0) MB, 0.4 / 0 ms [allocation failure].
[20822:0x2524d30]       60 ms: Scavenge 3.6 (39.0) -> 3.4 (40.0) MB, 0.4 / 0 ms [allocation failure].
[20822:0x2524d30]      161 ms: Scavenge 24.4 (59.1) -> 25.1 (60.1) MB, 1.6 / 0 ms [allocation failure].
[20822:0x2524d30]      161 ms: Scavenge 25.1 (60.1) -> 24.1 (61.1) MB, 0.7 / 0 ms [allocation failure].
[20822:0x2524d30]      231 ms: Scavenge 28.1 (61.1) -> 26.1 (61.1) MB, 0.3 / 0 ms [allocation failure].
[20822:0x2524d30]      253 ms: Scavenge 28.1 (61.1) -> 26.1 (61.1) MB, 0.2 / 0 ms [allocation failure].
{ rss: 74768384, heapTotal: 46019072, heapUsed: 37913336 }
[20822:0x2524d30]      329 ms: Mark-sweep 36.5 (72.1) -> 4.0 (43.0) MB, 4.2 / 0 ms [external memory allocation limit reached.] [GC in old space requested].
{ rss: 53116928, heapTotal: 24742400, heapUsed: 4239352 }
[20822:0x2524d30]      334 ms: Mark-sweep 4.1 (43.0) -> 4.0 (42.0) MB, 3.0 / 0 ms [external memory allocation limit reached.] [GC in old space requested].
{ rss: 45105152, heapTotal: 23710464, heapUsed: 4229864 }
[20822:0x2524d30]      337 ms: Mark-sweep 4.1 (42.0) -> 4.0 (42.0) MB, 2.4 / 0 ms [external memory allocation limit reached.] [GC in old space requested].
[20822:0x2524d30]      397 ms: Mark-sweep 4.0 (42.0) -> 3.7 (42.0) MB, 2.7 / 0 ms [external memory allocation limit reached.] [GC in old space requested].
[20822:0x2524d30]      401 ms: Mark-sweep 3.7 (42.0) -> 3.7 (42.0) MB, 2.2 / 0 ms [external memory allocation limit reached.] [GC in old space requested].

./node_v46 --trace_gc test/parallel/test-stringbytes-external

[20789:0x1319c10]       30 ms: Scavenge 2.1 (38.0) -> 1.9 (38.0) MB, 0.4 / 0 ms [allocation failure].
[20789:0x1319c10]       30 ms: Scavenge 2.1 (38.0) -> 2.1 (39.0) MB, 0.4 / 0 ms [allocation failure].
[20789:0x1319c10]       57 ms: Scavenge 3.6 (39.0) -> 3.3 (40.0) MB, 0.4 / 0 ms [allocation failure].
[20789:0x1319c10]      160 ms: Scavenge 24.4 (59.1) -> 25.1 (60.1) MB, 1.6 / 0 ms [allocation failure].
[20789:0x1319c10]      161 ms: Scavenge 25.1 (60.1) -> 24.1 (61.1) MB, 0.7 / 0 ms [allocation failure].
[20789:0x1319c10]      232 ms: Scavenge 28.1 (61.1) -> 26.1 (61.1) MB, 0.3 / 0 ms [allocation failure].
[20789:0x1319c10]      254 ms: Scavenge 28.2 (61.1) -> 26.2 (61.1) MB, 0.2 / 0 ms [allocation failure].
{ rss: 74747904, heapTotal: 46019072, heapUsed: 37971792 }
{ rss: 75018240, heapTotal: 47038976, heapUsed: 38365864 }
{ rss: 75018240, heapTotal: 47038976, heapUsed: 38394200 }
assert.js:311
    throw actual;
    ^

RangeError: Invalid array buffer length
    at new ArrayBuffer (native)
    at new Uint8Array (native)
    at allocate (buffer.js:98:17)
    at new Buffer (buffer.js:49:12)
    at /home/skomski/Code/node/test/parallel/test-stringbytes-external.js:145:5
    at Function._throws (assert.js:293:5)
    at Function.assert.throws (assert.js:319:11)
    at /home/skomski/Code/node/test/parallel/test-stringbytes-external.js:144:10
    at Object.<anonymous> (/home/skomski/Code/node/test/parallel/test-stringbytes-external.js:182:3)
    at Module._compile (module.js:434:26)
[1]    20789 exit 1     ./node_v46 --trace_gc test/parallel/test-stringbytes-external

targos · 2015-09-26T22:28:01Z

It may be related to the fact that 31450fc and 2b8a06b are not on this branch. Problem is that I cannot apply @indutny's patches nor the ones from v8.

trevnorris · 2015-09-26T22:39:32Z

@targos Do those two commits live on 4.7?

targos · 2015-09-26T22:53:12Z

Yes but with a different diff. See for example 2b8a06b vs v8/v8@0d01728.
I tried to mimic the changes manually but it doesn't work...

skomski · 2015-09-26T22:56:30Z

Already tested with vanilla v8 4.7 master with the same result as the custom v4.6 branch.

targos · 2015-09-29T14:57:19Z

@indutny do you know if we need to have 31450fc 2b8a06b in 4.6 ? If so, can we easily do it ?

indutny · 2015-09-29T15:05:47Z

@targos yeah, we need them. I can backport them by hand. Do you want me to do it?

targos · 2015-09-29T15:12:26Z

Yes please. I tried to do it but had compilation issues. You will probably know what to do more than me since you wrote the original patches.

indutny · 2015-09-29T15:54:04Z

Pushed two commits to vee-eight-4.6.

bnoordhuis · 2015-09-29T16:21:47Z

Without review? The Reviewed-By tags are lies now.

indutny · 2015-09-29T16:27:21Z

@bnoordhuis this is a staging branch, right? I suppose someone will add them when merging to real branch

ofrobots · 2015-09-29T16:50:20Z

IMO, it would be better to do reviews on vee-eight-* branches too. This improves collaboration and makes it much faster to land on master when the the merge time comes.

indutny · 2015-09-29T16:58:42Z

@ofrobots well, all previous commits wasn't reviewed, so I did it the same way as it was.

indutny · 2015-09-29T16:58:59Z

Guess we may put comments on commits.

targos · 2015-09-29T18:56:55Z

I agree that it would be better to have a proper review process for vee-eight-* branches. There are a few situations that we need to decide how to deal with:

Initiate the branch

I suggest to cut it from master after the previous one has been merged and start with a PR to upgrade V8 to the next version.

Keep the branch up to date with `master`

Do we merge master into it or do we regularly rebase on master ? I am for the second option but in this case how do we review a conflicting rebase ?

Keep V8 up to date in the branch

The beta branch of V8 usually has updates more often than the stable one. Do we make a PR for each one of them ?

ofrobots · 2015-09-30T14:46:51Z

Do we merge master into it or do we regularly rebase on master ? I am for the second option but in this case how do we review a conflicting rebase ?

I'd vote for rebase as well. See also previous discussion for the 4.5 branch. If there is a commit added to resolve a conflict during rebase, that gets reviewed as well.

The beta branch of V8 usually has updates more often than the stable one. Do we make a PR for each one of them ?

I don't think we have to have a PR for each one of them. I don't think the goal is to review V8 changes but rather to review us picking up those changes at whatever frequency makes sense for us.

On case insensitive platforms, the rule catches the debug module under npm and eslint and a source directory in next versions of V8. Do the same with the Release directory for consistency. Ref: nodejs#2286 (comment) Ref: nodejs#2688 (comment) PR-URL: nodejs#3144 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Roman Reiss <[email protected]>

targos · 2015-10-05T14:58:52Z

Rebased on master, updated V8 to 4.6.85.23 and cherry-picked all backports.
CI: https://ci.nodejs.org/job/node-test-commit/725/

targos · 2015-10-05T15:38:03Z

794 - test-util-inspect.js

not ok 794 test-util-inspect.js
# 
# undefined:1
# [Debug, ObjectIsPromise]
# ^
# ReferenceError: ObjectIsPromise is not defined
# at <anonymous>:1:9

@bnoordhuis It seems that ObjectIsPromise isn't exposed anymore in debug context :(

evanlucas · 2015-10-05T15:43:23Z

We could use v8::Value::IsPromise like #3119 is for Map/SetIterator

mgol · 2015-10-13T19:02:06Z

Chrome 46 is out so V8 4.6 is now stable.

targos · 2015-10-14T20:53:36Z

V8 4.6 is now on master.

targos added the v8 engine Issues and PRs related to the V8 dependency. label Sep 4, 2015

targos mentioned this issue Oct 1, 2015

gitignore: only ignore root Debug directory #3144

Closed

ofrobots mentioned this issue Oct 13, 2015

Land V8 4.6 onto master #3351

Closed

targos closed this as completed Oct 14, 2015

ChALkeR mentioned this issue Oct 15, 2015

Array spread produces empty array when used in object literal #2793

Closed

aliscco mentioned this issue Jun 21, 2023

[Snyk] Security upgrade @commitlint/cli from 8.2.0 to 8.3.6 aliscco/alisco-node#488

Open

aliscco mentioned this issue Apr 16, 2024

[Snyk] Security upgrade @commitlint/cli from 8.2.0 to 8.3.6 aliscco/alisco-node#1397

Open

Time to play with V8 4.6 #2688

Time to play with V8 4.6 #2688

Comments

targos commented Sep 4, 2015

indutny commented Sep 4, 2015

targos commented Sep 4, 2015

targos commented Sep 4, 2015

ofrobots commented Sep 4, 2015

Fishrock123 commented Sep 4, 2015

Fishrock123 commented Sep 4, 2015

kkoopa commented Sep 5, 2015

ChALkeR commented Sep 10, 2015

ChALkeR commented Sep 18, 2015

targos commented Sep 25, 2015

trevnorris commented Sep 25, 2015

targos commented Sep 26, 2015

targos commented Sep 26, 2015

targos commented Sep 26, 2015

targos commented Sep 26, 2015

trevnorris commented Sep 26, 2015

trevnorris commented Sep 26, 2015

targos commented Sep 26, 2015

trevnorris commented Sep 26, 2015

skomski commented Sep 26, 2015

targos commented Sep 26, 2015

trevnorris commented Sep 26, 2015

targos commented Sep 26, 2015

skomski commented Sep 26, 2015

targos commented Sep 29, 2015

indutny commented Sep 29, 2015

targos commented Sep 29, 2015

indutny commented Sep 29, 2015

bnoordhuis commented Sep 29, 2015

indutny commented Sep 29, 2015

ofrobots commented Sep 29, 2015

indutny commented Sep 29, 2015

indutny commented Sep 29, 2015

targos commented Sep 29, 2015

Initiate the branch

Keep the branch up to date with master

Keep V8 up to date in the branch

ofrobots commented Sep 30, 2015

targos commented Oct 5, 2015

targos commented Oct 5, 2015

evanlucas commented Oct 5, 2015

mgol commented Oct 13, 2015

targos commented Oct 14, 2015

Keep the branch up to date with `master`