-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
deps: cherry-pick d9fbfeb from upstream V8 #23886
Conversation
deps/v8/include/v8-version.h
Outdated
@@ -11,7 +11,7 @@ | |||
#define V8_MAJOR_VERSION 7 | |||
#define V8_MINOR_VERSION 0 | |||
#define V8_BUILD_NUMBER 276 | |||
#define V8_PATCH_LEVEL 28 | |||
#define V8_PATCH_LEVEL 29 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look right... 7.0.276.29 is: v8/v8@7.0.276.28...7.0.276.29
Please follow https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md#maintenance-process for backports. If V8 7.0 is an abandoned branch, then bump the v8_embedder_string
number in common.gypi
.
cc @nodejs/v8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, not sure why I thought this was the number to increment. I've changed it back and incremented the patch level in common.gypi.
I think @ak239 mentioned to me in the past that it would be better not to land this on V8 7.0.
6bf7c52
to
3c1afcc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RSLGTM
Let's see if this needs changes to node's test suite as well: |
Fails across the board for the V8 CI, e.g. https://ci.nodejs.org/job/node-test-commit-v8-linux/1791/nodes=benchmark,v8test=v8test/testReport/junit/(root)/v8tests/inspector_runtime_stable_object_id/
|
@ak239 do you know why these tests might be failing? Is there a pre-requisite change that would also need to get floated? |
Is work ongoing with this? (If so, it could use a rebase to get rid of the merge conflict.) |
@ak239 Thanks for the update! I'll go ahead and do so. |
e5bb4b7
to
bff312e
Compare
Travis is just quick sanity, we need validation from out CI cluster: |
bff312e
to
95733c2
Compare
Seems like there was a line I forgot to change. @refack, would you mind running it again? Also, do you know how I can run the V8 tests locally? |
If you look at the few first lines of the CI's console output ./configure
make -j 8 test-v8 V=1 DESTCPU=x64 ARCH=x64.release ENABLE_V8_TAP=True V8_EXTRA_TEST_OPTIONS=--progress=dots --timeout=120 Most importantly is |
95733c2
to
9951cb3
Compare
Original commit message: inspector: return [[StableObjectId]] as internal property This property might be useful for fast '===' check. R=dgozman@chromium.org,yangguo@chromium.org Bug: none Cq-Include-Trybots: luci.chromium.try:linux_chromium_headless_rel;luci.chromium.try:linux_chromium_rel_ng;master.tryserver.blink:linux_trusty_blink_rel Change-Id: Iabc3555ce1ec2c14cf0ccd40b7d964ae144e7352 Reviewed-on: https://chromium-review.googlesource.com/1226411 Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Reviewed-by: Yang Guo <yangguo@chromium.org> Reviewed-by: Jakob Gruber <jgruber@chromium.org> Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org> Cr-Commit-Position: refs/heads/master@{#56095}
9951cb3
to
215dfd7
Compare
@refack @ryzokuken @jasnell I believe this is ready to be landed... (pushes have just been to upgrade the V8 embedder string in |
Seems like landing V8 7.1 on master has superseded this PR... closing. |
Original commit message:
This commit allows us to determine the structure of circular objects through inspector (before, there was no surefire way to know if you entered an infinite loop while recursively iterating through an object's properties, because each object returned through the inspector was given a unique, monotonically increasing ID.
Hopefully, this change can make it into v10 and v11 at the very least.
cc/ @ak239 @ofrobots
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes