Bring in 5.1 - 5.0 abi compatibility #23

matthewloring · 2016-05-17T23:02:44Z

No description provided.

ofrobots · 2016-05-20T16:53:05Z

Good work! Looking at the diff from v8.h in 5.0, there is still this ABI difference remaining:

@@ -7149,7 +7323,7 @@ class Internals {
       1 * kApiPointerSize + kApiIntSize;
   static const int kStringResourceOffset = 3 * kApiPointerSize;

-  static const int kOddballKindOffset = 4 * kApiPointerSize;
+  static const int kOddballKindOffset = 5 * kApiPointerSize;
   static const int kForeignAddressOffset = kApiPointerSize;

matthewloring · 2016-05-20T22:19:43Z

That should be fixed now.

ofrobots · 2016-05-21T05:36:06Z

Looking at some of the other include/ files, a few more differences. These should be easy to fix however:

--- a/deps/v8/include/v8-debug.h
+++ b/deps/v8/include/v8-debug.h
@@ -18,13 +18,11 @@ enum DebugEvent {
   Exception = 2,
   NewFunction = 3,
   BeforeCompile = 4,
-  AfterCompile  = 5,
+  AfterCompile = 5,
   CompileError = 6,
-  PromiseEvent = 7,
-  AsyncTaskEvent = 8,
+  AsyncTaskEvent = 7,
 };
--- a/deps/v8/include/v8-platform.h
+++ b/deps/v8/include/v8-platform.h
@@ -152,9 +152,9 @@ class Platform {
    */
   virtual uint64_t AddTraceEvent(
       char phase, const uint8_t* category_enabled_flag, const char* name,
-      uint64_t id, uint64_t bind_id, int32_t num_args, const char** arg_names,
-      const uint8_t* arg_types, const uint64_t* arg_values,
-      unsigned int flags) {
+      const char* scope, uint64_t id, uint64_t bind_id, int32_t num_args,
+      const char** arg_names, const uint8_t* arg_types,
+      const uint64_t* arg_values, unsigned int flags) {
     return 0;
   }

matthewloring · 2016-05-21T19:56:01Z

These should now be compatible.

verwaest-zz · 2016-05-23T13:59:24Z

deps/v8/src/objects.h

  JS_MESSAGE_OBJECT_TYPE,
  JS_DATE_TYPE,
-  JS_OBJECT_TYPE,


Swapping this back around will be bad for performance, since we perform range checks on LAST_SPECIAL_RECEIVER_TYPE and LAST_CUSTOM_ELEMENTS_RECEIVER. If JS_OBJECT_TYPE falls above, all JS_OBJECT_TYPE objects (most common JavaScript object) will take the slow path for e.g., .hasOwnProperty.

matthewloring · 2016-05-24T14:39:45Z

I added custom checks for JS_OBJECT_TYPE at the comparison points to LAST_CUSTOM_ELEMENTS_RECEIVER. Using this contrived benchmark:

var o = {};
for (var i = 0; i < 10000000; i++) {
  Object.assign(o, { i: i });
  for (var k in o) {
    if (!o.hasOwnProperty(k)) {
      console.log('fail');
    }
  }
}

I observed:

mattloring-macbookpro:node mattloring$ time node-6.1.0 ~/Desktop/slow-test.js

real    0m1.985s
user    0m1.961s
sys 0m0.019s
mattloring-macbookpro:node mattloring$ time out/Release/node-old ~/Desktop/slow-test.js

real    0m25.168s
user    0m24.408s
sys 0m0.095s
mattloring-macbookpro:node mattloring$ time out/Release/node-fast ~/Desktop/slow-test.js

real    0m2.104s
user    0m1.960s
sys 0m0.016s

@ofrobots @verwaest What do you think?

ofrobots · 2016-05-25T18:43:54Z

deps/v8/src/builtins.cc

                                             LAST_CUSTOM_ELEMENTS_RECEIVER)),
+                          assembler->Word32NotEqual(
+                              instance_type, assembler->Int32Constant(
+                                             JS_OBJECT_TYPE))),


I think this is not right. JS_SPECIAL_API_OBJECT_TYPE used to be <= LAST_CUSTOM_ELEMENTS_RECEIVER before your change, but is > after your change. JS_OBJECT_TYPE is invariant here.

I think the expression you want is:

OR ( LESS_EQUAL (instance_type, LAST_CUSTOM_ELEMENTS_RECEIVER EQUAL(instance_type, JS_SPECIAL_API_OBJECT_TYPE))

ofrobots · 2016-05-27T13:56:29Z

LGTM to me. Let's get @verwaest to review as well.

targos · 2016-05-31T09:25:17Z

There is now v8/v8@ea5e96f in the 5.1 branch. Do you have to make changes for that?

verwaest-zz · 2016-05-31T18:18:57Z

LGTM as well.

matthewloring · 2016-05-31T19:21:32Z

I've updated the patch to support v8/v8@ea5e96f.

ofrobots · 2016-06-01T00:18:30Z

LGTM. @jeisinger PTAL as well.

jeisinger · 2016-06-02T06:42:19Z

LGTM

targos · 2016-06-02T15:39:19Z

@matthewloring If everything is ready, could you squash the commits so I can pick them up in nodejs#7016 ?

matthewloring · 2016-06-02T18:27:32Z

@targos All squashed.

targos · 2016-06-03T11:57:14Z

The patch does not apply cleanly on my branch

ofrobots · 2016-06-03T21:27:27Z

Actually this PR needs more work. Intercepted enumerator tests are failing for me.

@matthewloring I think the problem is that JS_SPECIAL_API_OBJECT_TYPE has been reordered wrt. LAST_CUSTOM_ELEMENTS_RECEIVER. API objects (with interceptors) no longer look like they require custom elements iteration breaking enumerator tests.

@targos: this is not likely to be ready before next week.

ofrobots · 2016-06-06T00:35:23Z

deps/v8/include/v8-debug.h

@@ -20,7 +20,8 @@ enum DebugEvent {
  BeforeCompile = 4,
  AfterCompile = 5,
  CompileError = 6,
-  AsyncTaskEvent = 7,
+  PromiseEvent = 7,


Looking at this more closely, I think the removal of PromiseEvent was actually an API change that is missing from the official list (/cc @natorion). I think we will have bring this back as a revert of https://codereview.chromium.org/1833563002.

ofrobots · 2016-06-06T20:22:29Z

deps/v8/src/builtins.cc

@@ -223,7 +223,8 @@ inline bool PrototypeHasNoElements(Isolate* isolate, JSObject* object) {
  HeapObject* empty = isolate->heap()->empty_fixed_array();
  while (prototype != null) {
    Map* map = prototype->map();
-    if (map->instance_type() <= LAST_CUSTOM_ELEMENTS_RECEIVER) return false;
+    if (map->instance_type() <= LAST_CUSTOM_ELEMENTS_RECEIVER ||
+      map->instance_type() == JS_GLOBAL_PROXY_TYPE) return false;


nit: align with the previous line.

The removal of the promise debug event is an API/ABI breaking change. https://codereview.chromium.org/1833563002

ofrobots · 2016-06-07T00:20:30Z

LGTM from my side. Given that that the roots list was reordered; it would be good to get signoff from @verwaest or @jeisinger as well.

jeisinger · 2016-06-07T15:04:34Z

lgtm

ofrobots · 2016-06-07T16:13:26Z

Launched a CI: https://ci.nodejs.org/job/node-test-commit/3684/
@targos this is rebased on top of your branch.

The removal of the promise debug event is an API/ABI breaking change. Ref: https://codereview.chromium.org/1833563002 Ref: ofrobots#23 PR-URL: nodejs#7016 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Ref: ofrobots#23 PR-URL: nodejs#7016 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

The removal of the promise debug event is an API/ABI breaking change. Ref: https://codereview.chromium.org/1833563002 Ref: ofrobots#23 PR-URL: #7016 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Ref: ofrobots#23 PR-URL: #7016 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

matthewloring · 2016-06-29T20:46:09Z

Closing this as it has landed in core.

The removal of the promise debug event is an API/ABI breaking change. Ref: https://codereview.chromium.org/1833563002 Ref: #23 PR-URL: nodejs#7016 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Ref: #23 PR-URL: nodejs#7016 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

verwaest-zz reviewed May 23, 2016
View reviewed changes

ofrobots reviewed May 25, 2016
View reviewed changes

ofrobots mentioned this pull request May 27, 2016

deps: update V8 to 5.1.281.69 nodejs/node#7016

Closed

2 tasks

ofrobots mentioned this pull request May 31, 2016

v8 5.1.281.47 issue with Nan::Null() nodejs/nan#578

Closed

bnoordhuis mentioned this pull request May 31, 2016

Does make sense to upgrade V8 to 5.1? nodejs/node#6354

Closed

ofrobots reviewed Jun 6, 2016
View reviewed changes

ofrobots force-pushed the vee-eight-5.1 branch from 098d365 to 8b713df Compare June 6, 2016 20:21

ofrobots reviewed Jun 6, 2016
View reviewed changes

Matt Loring added 2 commits June 6, 2016 17:13

deps: revert removal of V8::PromiseEvent

e9e07f9

The removal of the promise debug event is an API/ABI breaking change. https://codereview.chromium.org/1833563002

Bring in 5.1 - 5.0 abi compatibility

45a3b1e

targos pushed a commit to targos/node that referenced this pull request Jun 29, 2016

deps: bring in V8 5.1 - 5.0 ABI compatibility

d79df45

Ref: ofrobots#23 PR-URL: nodejs#7016 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

targos pushed a commit to nodejs/node that referenced this pull request Jun 29, 2016

deps: bring in V8 5.1 - 5.0 ABI compatibility

9beef23

Ref: ofrobots#23 PR-URL: #7016 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

matthewloring closed this Jun 29, 2016

ofrobots pushed a commit that referenced this pull request Aug 25, 2016

deps: bring in V8 5.1 - 5.0 ABI compatibility

2c46e23

Ref: #23 PR-URL: nodejs#7016 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

matthewloring deleted the abi branch October 20, 2016 21:14

matthewloring mentioned this pull request Nov 7, 2016

Make V8 5.5 ABI compatible with 5.4 nodejs/v8#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring in 5.1 - 5.0 abi compatibility #23

Bring in 5.1 - 5.0 abi compatibility #23

matthewloring commented May 17, 2016

ofrobots commented May 20, 2016 •

edited

Loading

matthewloring commented May 20, 2016

ofrobots commented May 21, 2016 •

edited

Loading

matthewloring commented May 21, 2016

verwaest-zz May 23, 2016

matthewloring commented May 24, 2016

ofrobots May 25, 2016 •

edited

Loading

matthewloring May 25, 2016

ofrobots commented May 27, 2016

targos commented May 31, 2016

verwaest-zz commented May 31, 2016

matthewloring commented May 31, 2016

ofrobots commented Jun 1, 2016

jeisinger commented Jun 2, 2016

targos commented Jun 2, 2016

matthewloring commented Jun 2, 2016

targos commented Jun 3, 2016

ofrobots commented Jun 3, 2016

ofrobots Jun 6, 2016

ofrobots Jun 6, 2016

ofrobots commented Jun 7, 2016

jeisinger commented Jun 7, 2016

ofrobots commented Jun 7, 2016

matthewloring commented Jun 29, 2016

Bring in 5.1 - 5.0 abi compatibility #23

Bring in 5.1 - 5.0 abi compatibility #23

Conversation

matthewloring commented May 17, 2016

ofrobots commented May 20, 2016 • edited Loading

matthewloring commented May 20, 2016

ofrobots commented May 21, 2016 • edited Loading

matthewloring commented May 21, 2016

verwaest-zz May 23, 2016

Choose a reason for hiding this comment

matthewloring commented May 24, 2016

ofrobots May 25, 2016 • edited Loading

Choose a reason for hiding this comment

matthewloring May 25, 2016

Choose a reason for hiding this comment

ofrobots commented May 27, 2016

targos commented May 31, 2016

verwaest-zz commented May 31, 2016

matthewloring commented May 31, 2016

ofrobots commented Jun 1, 2016

jeisinger commented Jun 2, 2016

targos commented Jun 2, 2016

matthewloring commented Jun 2, 2016

targos commented Jun 3, 2016

ofrobots commented Jun 3, 2016

ofrobots Jun 6, 2016

Choose a reason for hiding this comment

ofrobots Jun 6, 2016

Choose a reason for hiding this comment

ofrobots commented Jun 7, 2016

jeisinger commented Jun 7, 2016

ofrobots commented Jun 7, 2016

matthewloring commented Jun 29, 2016

ofrobots commented May 20, 2016 •

edited

Loading

ofrobots commented May 21, 2016 •

edited

Loading

ofrobots May 25, 2016 •

edited

Loading