Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2536463002:
Create JSPromise (Closed)
DescriptionObject
-- New JSObject for promises: JSPromise
Builtins
-- PromiseThen TFJ
-- PromiseCreateAndSet TFJ for internal use
-- PerformPromiseThen TFJ for internal use
-- PromiseInit for initial promise setup
-- SpeciesConstructor for use in PromiseThen
-- ThrowIfNotJSReceiver for use in SpeciesConstructor
-- AppendPromiseCallback to update FixedArray with new callback
-- InternalPerformPromiseThen
Promises.js
-- Cleanup unused symbols
-- Remove PerformPromiseThen
-- Remove PromiseThen
-- Remove PromiseSet
-- Remove PromiseAttachCallbacks
Runtime
-- PromiseSet to set promise inobject values
-- Refactor functions to use FixedArrays for callbacks instead of
JSArray
-- Runtime_PromiseStatus to return promise status
-- Runtime_PromiseResult to return promise result
-- Runtime_PromiseDeferred to return deferred attached to promise
-- Runtime_PromiseRejectReactions to return reject reactions attached
to promise
This CL results in a 13.07% improvement in the promises benchmark
(over 5 runs).
BUG=v8:5343
Committed: https://crrev.com/30b564c76f490f8f6b311a74b25b26cf0a96be2d
Cr-Commit-Position: refs/heads/master@{#41503}
Patch Set 1 #Patch Set 2 : work work work #Patch Set 3 : set dependent patch #Patch Set 4 : work work work #Patch Set 5 : fix #Patch Set 6 : Remove stuff #Patch Set 7 : update PromiseFulfill arg count #
Total comments: 85
Patch Set 8 : fix review comments #Patch Set 9 : work work work #Patch Set 10 : rebase #Patch Set 11 : rebase #Patch Set 12 : rebase #Patch Set 13 : use update write barrier #
Total comments: 1
Patch Set 14 : rebase #Patch Set 15 : use mode #Patch Set 16 : rebase #
Messages
Total messages: 76 (58 generated)
Description was changed from ========== move PromiseThen to tfj ========== to ========== move PromiseThen to tfj BUG= ==========
Description was changed from ========== move PromiseThen to tfj BUG= ========== to ========== Object -- New JSObject for promises: JSPromise Builtins -- PromiseThen TFJ -- PromiseCreateAndSet TFJ for internal use -- PerformPromiseThen TFJ for internal use -- PromiseInit for initial promise setup -- SpeciesConstructor for use in PromiseThen -- ThrowIfNotJSReceiver for use in SpeciesConstructor -- AppendPromiseCallback to update FixedArray with new callback -- InternalPerformPromiseThen Promises.js -- Cleanup unused symbols -- Remove PerformPromiseThen -- Remove PromiseThen -- Remove PromiseSet -- Remove PromiseAttachCallbacks Runtime -- PromiseSet to set promise inobject values -- Refactor functions to use FixedArrays for callbacks instead of JSArray -- Runtime_PromiseStatus to return promise status -- Runtime_PromiseResult to return promise result -- Runtime_PromiseDeferred to return deferred attached to promise -- Runtime_PromiseRejectReactions to return reject reactions attached to promise BUG=v8:5343 ==========
Description was changed from ========== Object -- New JSObject for promises: JSPromise Builtins -- PromiseThen TFJ -- PromiseCreateAndSet TFJ for internal use -- PerformPromiseThen TFJ for internal use -- PromiseInit for initial promise setup -- SpeciesConstructor for use in PromiseThen -- ThrowIfNotJSReceiver for use in SpeciesConstructor -- AppendPromiseCallback to update FixedArray with new callback -- InternalPerformPromiseThen Promises.js -- Cleanup unused symbols -- Remove PerformPromiseThen -- Remove PromiseThen -- Remove PromiseSet -- Remove PromiseAttachCallbacks Runtime -- PromiseSet to set promise inobject values -- Refactor functions to use FixedArrays for callbacks instead of JSArray -- Runtime_PromiseStatus to return promise status -- Runtime_PromiseResult to return promise result -- Runtime_PromiseDeferred to return deferred attached to promise -- Runtime_PromiseRejectReactions to return reject reactions attached to promise BUG=v8:5343 ========== to ========== Object -- New JSObject for promises: JSPromise Builtins -- PromiseThen TFJ -- PromiseCreateAndSet TFJ for internal use -- PerformPromiseThen TFJ for internal use -- PromiseInit for initial promise setup -- SpeciesConstructor for use in PromiseThen -- ThrowIfNotJSReceiver for use in SpeciesConstructor -- AppendPromiseCallback to update FixedArray with new callback -- InternalPerformPromiseThen Promises.js -- Cleanup unused symbols -- Remove PerformPromiseThen -- Remove PromiseThen -- Remove PromiseSet -- Remove PromiseAttachCallbacks Runtime -- PromiseSet to set promise inobject values -- Refactor functions to use FixedArrays for callbacks instead of JSArray -- Runtime_PromiseStatus to return promise status -- Runtime_PromiseResult to return promise result -- Runtime_PromiseDeferred to return deferred attached to promise -- Runtime_PromiseRejectReactions to return reject reactions attached to promise This CL results in a 13.07% improvement in the promises benchmark (over 5 runs). BUG=v8:5343 ==========
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Object -- New JSObject for promises: JSPromise Builtins -- PromiseThen TFJ -- PromiseCreateAndSet TFJ for internal use -- PerformPromiseThen TFJ for internal use -- PromiseInit for initial promise setup -- SpeciesConstructor for use in PromiseThen -- ThrowIfNotJSReceiver for use in SpeciesConstructor -- AppendPromiseCallback to update FixedArray with new callback -- InternalPerformPromiseThen Promises.js -- Cleanup unused symbols -- Remove PerformPromiseThen -- Remove PromiseThen -- Remove PromiseSet -- Remove PromiseAttachCallbacks Runtime -- PromiseSet to set promise inobject values -- Refactor functions to use FixedArrays for callbacks instead of JSArray -- Runtime_PromiseStatus to return promise status -- Runtime_PromiseResult to return promise result -- Runtime_PromiseDeferred to return deferred attached to promise -- Runtime_PromiseRejectReactions to return reject reactions attached to promise This CL results in a 13.07% improvement in the promises benchmark (over 5 runs). BUG=v8:5343 ========== to ========== Object -- New JSObject for promises: JSPromise Builtins -- PromiseThen TFJ -- PromiseCreateAndSet TFJ for internal use -- PerformPromiseThen TFJ for internal use -- PromiseInit for initial promise setup -- SpeciesConstructor for use in PromiseThen -- ThrowIfNotJSReceiver for use in SpeciesConstructor -- AppendPromiseCallback to update FixedArray with new callback -- InternalPerformPromiseThen Promises.js -- Cleanup unused symbols -- Remove PerformPromiseThen -- Remove PromiseThen -- Remove PromiseSet -- Remove PromiseAttachCallbacks Runtime -- PromiseSet to set promise inobject values -- Refactor functions to use FixedArrays for callbacks instead of JSArray -- Runtime_PromiseStatus to return promise status -- Runtime_PromiseResult to return promise result -- Runtime_PromiseDeferred to return deferred attached to promise -- Runtime_PromiseRejectReactions to return reject reactions attached to promise This CL results in a 13.07% improvement in the promises benchmark (over 5 runs). BUG=v8:5343 ==========
gsathya@chromium.org changed reviewers: + bmeurer@chromium.org, caitp@igalia.com, jgruber@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17111) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Have some inline questions about FixedArrays in builtins-promise.cc::AppendPromiseCallback
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
I haven't looked too closely yet, but I like this better than the approach I was going with. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (left): https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:223: void Builtins::Generate_IsPromise(compiler::CodeAssemblerState* state) { Is there a way to handle native-context stored intrinsics in the compiler backends? Like, if they have a BuiltinFunctionId, could these be inlined in the interpreter to avoid the jump? It looks like interpreter-intrinsics.cc can't really do this, and there doesn't seem to be a precedent for it in turbofan. Not that big of a deal, but it would be cool to get rid of the call overhead https://codereview.chromium.org/2536463002/diff/120001/src/heap-symbols.h File src/heap-symbols.h (left): https://codereview.chromium.org/2536463002/diff/120001/src/heap-symbols.h#old... src/heap-symbols.h:221: V(promise_has_handler_symbol) \ Maybe we should include the has_handler symbol as an inobject field, since it's stored fairly commonly? It looks like this is doing a similar job to the [[PromiseIsHandled]] internal field in the spec https://codereview.chromium.org/2536463002/diff/120001/src/js/async-await.js File src/js/async-await.js (right): https://codereview.chromium.org/2536463002/diff/120001/src/js/async-await.js#... src/js/async-await.js:45: if (%is_promise(value)) { As long as we're making a change to this line, I think we should add a TODO about the spec noncompliant behaviour https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js#newc... src/js/promise.js:139: var thenableState = %PromiseStatus(resolution); We should really inline the status getter, at least in ignition. interpreter-intrinsics.cc can definitely do that https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js#newc... src/js/promise.js:144: %PromiseFulfill(promise, kFulfilled, thenableValue); Why don't we just do both of these in a single runtime call? Since it's an internal field now, loading the value from C++ is cheap, and we avoid jumping back and forth between C++ and the runtime again https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js#newc... src/js/promise.js:408: var deferred = %PromiseDeferred(promise); And yeah, we should really inline these accessors https://codereview.chromium.org/2536463002/diff/120001/src/objects-inl.h File src/objects-inl.h (right): https://codereview.chromium.org/2536463002/diff/120001/src/objects-inl.h#newc... src/objects-inl.h:7095: ACCESSORS(JSPromise, reject_reactions, Object, kRejectReactionsOffset) You probably need a change to int JSObject::GetHeaderSize(InstanceType type) in this file, too. It looks like it affects EnsureHasInitialMap(). But, it may not actually matter in this case. https://codereview.chromium.org/2536463002/diff/120001/src/objects-printer.cc File src/objects-printer.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/objects-printer.cc... src/objects-printer.cc:547: os << "\n - status = " << Brief(status()); I think it would be easier to understand when reading this if the status were stringified
these get all the failing tests passing for me locally, except for that failing inspector test (but that may just be due to the missing *-expected.txt file? https://codereview.chromium.org/2536463002/diff/120001/src/objects-debug.cc File src/objects-debug.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/objects-debug.cc#n... src/objects-debug.cc:889: CHECK(fulfill_reactions()->IsUndefined(isolate) || Since fulfill_reactions() and reject_reactions() are FixedArrays now instead of JSArrays, PromiseReactionJobInfoVerify() needs an update. Looks like the deferred() field can be a FixedArray, too. https://codereview.chromium.org/2536463002/diff/120001/src/objects-debug.cc#n... src/objects-debug.cc:890: fulfill_reactions()->IsJSFunction() || This should be IsCallable(), since bound functions don't pass the IsJSFunction() test (and n'either do callable Proxies). Ditto for the reject handlers https://codereview.chromium.org/2536463002/diff/120001/src/objects-printer.cc File src/objects-printer.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/objects-printer.cc... src/objects-printer.cc:154: JSPromise::cast(this)->JSPromisePrint(os); All those cases that are going to fall through down to this one are going to crash, we need to move the JS_PROMISE_TYPE case somewhere else
https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:397: a->CopyFixedArrayElements(kind, elements, new_elements, length, I think this can end up in LargeObjectSpace if you append enough callbacks. The comment above LargeObjectSpace seems to indicate that it should be safe to write to without a barrier, but I'm not sure. Even if lospace is safe without a barrier, I thought the main reason for the barrier was related to the source operand based on comments in other CLs ("you need a write barrier since you're not storing a strongly rooted object").
https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:397: a->CopyFixedArrayElements(kind, elements, new_elements, length, On 2016/12/01 15:31:44, caitp wrote: > I think this can end up in LargeObjectSpace if you append enough callbacks. The > comment above LargeObjectSpace seems to indicate that it should be safe to write > to without a barrier, but I'm not sure. > > Even if lospace is safe without a barrier, I thought the main reason for the > barrier was related to the source operand based on comments in other CLs ("you > need a write barrier since you're not storing a strongly rooted object"). ```` So given that, I think we need to be more careful about how we decide which space the new array is allocated in. Similar to the GrowElementsCapacity() helper in CodeStubASsembler var resolve, reject, p = new Promise(function(res, rej) { resolve = res; reject = rej; }); var arrayScript = "[" + ("0,".repeat(2000000)) + "]"; // fill up new space var a = eval(arrayScript); for (var i = 0; i < (65536); ++i) { print("chaining #" + i); p.then(function(v) { print(v); return v + 1; }, function(e) { return e; }); a[1] = i; a[2000000 - 1] = i & (2000000 - 1); } p = p.then(x => print(x), e => print(e.stack)); resolve(0); ``` results in this crash: ``` # # Fatal error in ../../src/runtime/runtime-internal.cc, line 264 # Check failed: size <= kMaxRegularHeapObjectSize. # ==== C stack trace =============================== 0 libv8_libbase.dylib 0x0000000100735dbe v8::base::debug::StackTrace::StackTrace() + 30 1 libv8_libbase.dylib 0x0000000100735df5 v8::base::debug::StackTrace::StackTrace() + 21 2 libv8_libbase.dylib 0x000000010072b274 V8_Fatal + 452 3 libv8.dylib 0x0000000102bc685f v8::internal::__RT_impl_Runtime_AllocateInNewSpace(v8::internal::Arguments, v8::internal::Isolate*) + 447 4 libv8.dylib 0x0000000102bc64ce v8::internal::Runtime_AllocateInNewSpace(int, v8::internal::Object**, v8::internal::Isolate*) + 286 5 ??? 0x00003cebb3504427 0x0 + 66983023363111 ```
Nice! First round of comments. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:86: a->StoreObjectField(promise, JSPromise::kStatusOffset, status); You could assert TaggedIsSmi(status) here. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:89: return; Please remove. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:257: compiler::Node* ThrowIfNotJSReceiver(CodeStubAssembler* a, Isolate* isolate, Maybe we could refactor the similar method in builtins-regexp to avoid duplication. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:292: // TODO(gsathya): Refactor promise.js::IsPromise to use this. Seems like you can remove this TODO. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:303: a.Branch(a.HasInstanceType(maybe_promise, JS_PROMISE_TYPE), &if_ispromise, Simpler: Node* const result = a.Select(a.HasInstanceType(maybe_promise, JS_PROMISE_TYPE), a.TrueConstant(), a.FalseConstant()) a.Return(result); https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:338: MessageTemplate::kConstructorNotReceiver); The kConstructorNotReceiver message template does not take an argument. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:366: a->CallRuntime(Runtime::kThrowTypeError, context, message_id, object); Same here. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:391: // Is this necessary? I don't think so. You copy [0,length[ below and then set new_elements[length], and there are no allocations in between, so filling with undefined in advance is just useless work. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:457: a->GotoUnless(a->WordEqual(status, a->SmiConstant(kPromisePending)), SmiEqual https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:519: AppendPromiseCallback(a, JSPromise::kDeferredOffset, promise, deferred); Not sure how common this case is, but if it happens often we could go with a better array growth strategy. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:570: return; Please remove. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:624: a.Branch(a.WordEqual(promise_fun, constructor), &fast_promise_capability, If we want to make this faster at some point, we could do an additional fast path check before 2. above. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:643: // TODO(gsathya): Move this to TF. I probably wouldn't worry too much about the slow path. https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js#newc... src/js/promise.js:416: for (var i = 0; i < queue.length; i += 1) { Nit: i++ https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js#newc... src/js/promise.js:417: if (PromiseHasUserDefinedRejectHandlerCheck(queue[i], deferred[i])) { Was this logic refactored in this CL? Is deferred guaranteed to be an array of the same length as queue? https://codereview.chromium.org/2536463002/diff/120001/src/objects-printer.cc File src/objects-printer.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/objects-printer.cc... src/objects-printer.cc:545: void JSPromise::JSPromisePrint(std::ostream& os) { // NOLINT How much do we gain through JSPromisePrint(promise) over JSObjectPrint(promise)? If the output is similar, we could just go with that. https://codereview.chromium.org/2536463002/diff/120001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2536463002/diff/120001/src/objects.h#newcode8836 src/objects.h:8836: // directly attached here. Otherwise, we use FixedArrays. Please be a bit more detailed here concerning possible contents of these fields (e.g. that fulfill_reactions is undefined/fixed_array/callable). https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-de... File src/runtime/runtime-debug.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-de... src/runtime/runtime-debug.cc:246: int status_val = promise->status()->value(); This could be just status() if you go with a smi accessor. https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-de... src/runtime/runtime-debug.cc:265: Handle<Object> value_obj = handle(promise->result(), isolate); value_obj(promise->result(), isolate) https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... File src/runtime/runtime-promise.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:4: #include "iostream" Can we remove this? https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:69: Handle<Object> deferred_obj = deferred; Nit: Handle<Object> deferred_obj(deferred); https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:77: isolate, isolate->promise_debug_get_info(), I don't understand how deferred_obj should interact with PromiseDebugGetInfo. If I'm reading this correctly, then HAS_PRIVATE(deferreds.promise, promiseHandledBySymbol) will always be false, and you could just as well pass in undefined instead of allocating a new JSArray. https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:98: void PromiseSet(Handle<JSPromise> promise, Handle<Smi> status, You could with 'Smi* status' or 'int status' here and in callers (and DECL_INT_ACCESSORS in objects.h). https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:111: if (status->value() == kPromiseFulfilled) { Maybe more efficient: tasks((status->value() == kPromiseFulfilled) ? promise->fulfill_reactions() : promise->reject_reactions(), isolate) https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:253: if (deferred->IsJSObject()) { Could you document when deferred is undefined / a JSObject / a FixedArray? The comment in object.h does not mention undefined, nor the contents when it is not undefined or a FixedArray.
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/17255) v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/18893)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13600) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/17297) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/18936) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (left): https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:223: void Builtins::Generate_IsPromise(compiler::CodeAssemblerState* state) { On 2016/12/01 01:36:50, caitp wrote: > Is there a way to handle native-context stored intrinsics in the compiler > backends? Like, if they have a BuiltinFunctionId, could these be inlined in the > interpreter to avoid the jump? It looks like interpreter-intrinsics.cc can't > really do this, and there doesn't seem to be a precedent for it in turbofan. > > Not that big of a deal, but it would be cool to get rid of the call overhead After the refactoring is done, only async-await.js will call this. Not sure if it's worth it. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:86: a->StoreObjectField(promise, JSPromise::kStatusOffset, status); On 2016/12/01 16:30:26, jgruber wrote: > You could assert TaggedIsSmi(status) here. Done. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:89: return; On 2016/12/01 16:30:27, jgruber wrote: > Please remove. Done. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:257: compiler::Node* ThrowIfNotJSReceiver(CodeStubAssembler* a, Isolate* isolate, On 2016/12/01 16:30:27, jgruber wrote: > Maybe we could refactor the similar method in builtins-regexp to avoid > duplication. Agreed, but this is slightly different. (Doesn't take a method_name) https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:292: // TODO(gsathya): Refactor promise.js::IsPromise to use this. On 2016/12/01 16:30:26, jgruber wrote: > Seems like you can remove this TODO. Done. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:303: a.Branch(a.HasInstanceType(maybe_promise, JS_PROMISE_TYPE), &if_ispromise, On 2016/12/01 16:30:27, jgruber wrote: > Simpler: > > Node* const result = a.Select(a.HasInstanceType(maybe_promise, JS_PROMISE_TYPE), > a.TrueConstant(), a.FalseConstant()) > a.Return(result); Nice! Done https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:338: MessageTemplate::kConstructorNotReceiver); On 2016/12/01 16:30:26, jgruber wrote: > The kConstructorNotReceiver message template does not take an argument. Not passing an argument here. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:366: a->CallRuntime(Runtime::kThrowTypeError, context, message_id, object); On 2016/12/01 16:30:26, jgruber wrote: > Same here. Done. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:391: // Is this necessary? On 2016/12/01 16:30:27, jgruber wrote: > I don't think so. You copy [0,length[ below and then set new_elements[length], > and there are no allocations in between, so filling with undefined in advance is > just useless work. Yeah, agreed. Removed https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:457: a->GotoUnless(a->WordEqual(status, a->SmiConstant(kPromisePending)), On 2016/12/01 16:30:27, jgruber wrote: > SmiEqual Done. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:519: AppendPromiseCallback(a, JSPromise::kDeferredOffset, promise, deferred); On 2016/12/01 16:30:26, jgruber wrote: > Not sure how common this case is, but if it happens often we could go with a > better array growth strategy. I bet 99% of all use cases just have a single callback. I don't think is a problem right now. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:570: return; On 2016/12/01 16:30:27, jgruber wrote: > Please remove. Done. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:624: a.Branch(a.WordEqual(promise_fun, constructor), &fast_promise_capability, On 2016/12/01 16:30:26, jgruber wrote: > If we want to make this faster at some point, we could do an additional fast > path check before 2. above. Faster how? By skipping the species constructor lookup? https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:643: // TODO(gsathya): Move this to TF. On 2016/12/01 16:30:26, jgruber wrote: > I probably wouldn't worry too much about the slow path. Not exactly for perf reasons, but because moving everything to TF/CPP. https://codereview.chromium.org/2536463002/diff/120001/src/heap-symbols.h File src/heap-symbols.h (left): https://codereview.chromium.org/2536463002/diff/120001/src/heap-symbols.h#old... src/heap-symbols.h:221: V(promise_has_handler_symbol) \ On 2016/12/01 01:36:50, caitp wrote: > Maybe we should include the has_handler symbol as an inobject field, since it's > stored fairly commonly? It looks like this is doing a similar job to the > [[PromiseIsHandled]] internal field in the spec Yeah, agreed. Will do in follow up patch here which touches this symbol more -- https://codereview.chromium.org/2541283002/ https://codereview.chromium.org/2536463002/diff/120001/src/js/async-await.js File src/js/async-await.js (right): https://codereview.chromium.org/2536463002/diff/120001/src/js/async-await.js#... src/js/async-await.js:45: if (%is_promise(value)) { On 2016/12/01 01:36:50, caitp wrote: > As long as we're making a change to this line, I think we should add a TODO > about the spec noncompliant behaviour Done. Assigned to you :P https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js#newc... src/js/promise.js:139: var thenableState = %PromiseStatus(resolution); On 2016/12/01 01:36:50, caitp wrote: > We should really inline the status getter, at least in ignition. > interpreter-intrinsics.cc can definitely do that This and the debug mirror code is the only place use this. This function is going away soon -- https://codereview.chromium.org/2541283002/ Not sure if it's worth the trouble for just the debug code. https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js#newc... src/js/promise.js:144: %PromiseFulfill(promise, kFulfilled, thenableValue); On 2016/12/01 01:36:50, caitp wrote: > Why don't we just do both of these in a single runtime call? Since it's an > internal field now, loading the value from C++ is cheap, and we avoid jumping > back and forth between C++ and the runtime again Yeah, this is hacky. This CL already touches too many things, didn't want to change this function as well. This is going away -- https://codereview.chromium.org/2541283002/ https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js#newc... src/js/promise.js:408: var deferred = %PromiseDeferred(promise); On 2016/12/01 01:36:50, caitp wrote: > And yeah, we should really inline these accessors But this is debug code, not sure if it's worth it. Maybe I'll just port all this debug code to c++, might be easier, then we can get rid of the runtime calls for these accessors. https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js#newc... src/js/promise.js:416: for (var i = 0; i < queue.length; i += 1) { On 2016/12/01 16:30:27, jgruber wrote: > Nit: i++ Done. https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js#newc... src/js/promise.js:417: if (PromiseHasUserDefinedRejectHandlerCheck(queue[i], deferred[i])) { On 2016/12/01 16:30:27, jgruber wrote: > Was this logic refactored in this CL? Is deferred guaranteed to be an array of > the same length as queue? Yes. Yes. They are always updated together in PerformPromiseThen. https://codereview.chromium.org/2536463002/diff/120001/src/objects-debug.cc File src/objects-debug.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/objects-debug.cc#n... src/objects-debug.cc:889: CHECK(fulfill_reactions()->IsUndefined(isolate) || On 2016/12/01 04:03:07, caitp wrote: > Since fulfill_reactions() and reject_reactions() are FixedArrays now instead of > JSArrays, PromiseReactionJobInfoVerify() needs an update. Looks like the > deferred() field can be a FixedArray, too. Ah, yes, good catch. Done https://codereview.chromium.org/2536463002/diff/120001/src/objects-debug.cc#n... src/objects-debug.cc:890: fulfill_reactions()->IsJSFunction() || On 2016/12/01 04:03:07, caitp wrote: > This should be IsCallable(), since bound functions don't pass the IsJSFunction() > test (and n'either do callable Proxies). Ditto for the reject handlers Done. https://codereview.chromium.org/2536463002/diff/120001/src/objects-inl.h File src/objects-inl.h (right): https://codereview.chromium.org/2536463002/diff/120001/src/objects-inl.h#newc... src/objects-inl.h:7095: ACCESSORS(JSPromise, reject_reactions, Object, kRejectReactionsOffset) On 2016/12/01 01:36:50, caitp wrote: > You probably need a change to int JSObject::GetHeaderSize(InstanceType type) in > this file, too. It looks like it affects EnsureHasInitialMap(). But, it may not > actually matter in this case. Done. https://codereview.chromium.org/2536463002/diff/120001/src/objects-printer.cc File src/objects-printer.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/objects-printer.cc... src/objects-printer.cc:154: JSPromise::cast(this)->JSPromisePrint(os); On 2016/12/01 04:03:07, caitp wrote: > All those cases that are going to fall through down to this one are going to > crash, we need to move the JS_PROMISE_TYPE case somewhere else Done. https://codereview.chromium.org/2536463002/diff/120001/src/objects-printer.cc... src/objects-printer.cc:545: void JSPromise::JSPromisePrint(std::ostream& os) { // NOLINT On 2016/12/01 16:30:27, jgruber wrote: > How much do we gain through JSPromisePrint(promise) over JSObjectPrint(promise)? > If the output is similar, we could just go with that. Printing out the status as strings could be useful as caitp points out. https://codereview.chromium.org/2536463002/diff/120001/src/objects-printer.cc... src/objects-printer.cc:547: os << "\n - status = " << Brief(status()); On 2016/12/01 01:36:50, caitp wrote: > I think it would be easier to understand when reading this if the status were > stringified Done. https://codereview.chromium.org/2536463002/diff/120001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2536463002/diff/120001/src/objects.h#newcode8836 src/objects.h:8836: // directly attached here. Otherwise, we use FixedArrays. On 2016/12/01 16:30:27, jgruber wrote: > Please be a bit more detailed here concerning possible contents of these fields > (e.g. that fulfill_reactions is undefined/fixed_array/callable). Done. https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-de... File src/runtime/runtime-debug.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-de... src/runtime/runtime-debug.cc:246: int status_val = promise->status()->value(); On 2016/12/01 16:30:27, jgruber wrote: > This could be just status() if you go with a smi accessor. Done. https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-de... src/runtime/runtime-debug.cc:265: Handle<Object> value_obj = handle(promise->result(), isolate); On 2016/12/01 16:30:27, jgruber wrote: > value_obj(promise->result(), isolate) Done. https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... File src/runtime/runtime-promise.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:4: #include "iostream" On 2016/12/01 16:30:27, jgruber wrote: > Can we remove this? oops, removed https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:69: Handle<Object> deferred_obj = deferred; On 2016/12/01 16:30:27, jgruber wrote: > Nit: Handle<Object> deferred_obj(deferred); Done. https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:77: isolate, isolate->promise_debug_get_info(), On 2016/12/01 16:30:27, jgruber wrote: > I don't understand how deferred_obj should interact with PromiseDebugGetInfo. If > I'm reading this correctly, then > > HAS_PRIVATE(deferreds.promise, promiseHandledBySymbol) > > will always be false, and you could just as well pass in undefined instead of > allocating a new JSArray. Yeah, agreed. Changed to Undefined. https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:98: void PromiseSet(Handle<JSPromise> promise, Handle<Smi> status, On 2016/12/01 16:30:27, jgruber wrote: > You could with 'Smi* status' or 'int status' here and in callers (and > DECL_INT_ACCESSORS in objects.h). Done. https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:111: if (status->value() == kPromiseFulfilled) { On 2016/12/01 16:30:27, jgruber wrote: > Maybe more efficient: > > tasks((status->value() == kPromiseFulfilled) ? > promise->fulfill_reactions() : > promise->reject_reactions(), isolate) Hmm, not sure if it's more readable tbh. Changed anyway https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:253: if (deferred->IsJSObject()) { On 2016/12/01 16:30:27, jgruber wrote: > Could you document when deferred is undefined / a JSObject / a FixedArray? The > comment in object.h does not mention undefined, nor the contents when it is not > undefined or a FixedArray. Updated comment in objects.h
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/17323) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13630) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/17327) v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13634) v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)
Still looking into the test failures. Seems related to the append code, but not reproducible on my machine ..
lgtm https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:257: compiler::Node* ThrowIfNotJSReceiver(CodeStubAssembler* a, Isolate* isolate, On 2016/12/04 17:36:17, gsathya wrote: > On 2016/12/01 16:30:27, jgruber wrote: > > Maybe we could refactor the similar method in builtins-regexp to avoid > > duplication. > > Agreed, but this is slightly different. (Doesn't take a method_name) Acknowledged. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:389: Node* new_elements = a->AllocateFixedArray(kind, new_capacity, mode); Mentioning this now because I started running into it with RegExp: this probably needs a check for kMaxRegularHeapObjectSize somewhere. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:519: AppendPromiseCallback(a, JSPromise::kDeferredOffset, promise, deferred); On 2016/12/04 17:36:17, gsathya wrote: > On 2016/12/01 16:30:26, jgruber wrote: > > Not sure how common this case is, but if it happens often we could go with a > > better array growth strategy. > > I bet 99% of all use cases just have a single callback. I don't think is a > problem right now. Acknowledged. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:624: a.Branch(a.WordEqual(promise_fun, constructor), &fast_promise_capability, On 2016/12/04 17:36:17, gsathya wrote: > On 2016/12/01 16:30:26, jgruber wrote: > > If we want to make this faster at some point, we could do an additional fast > > path check before 2. above. > > Faster how? By skipping the species constructor lookup? Actually, thinking about this again, that's probably not a good idea after all since changing the constructor property does not change the map. https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:643: // TODO(gsathya): Move this to TF. On 2016/12/04 17:36:17, gsathya wrote: > On 2016/12/01 16:30:26, jgruber wrote: > > I probably wouldn't worry too much about the slow path. > > Not exactly for perf reasons, but because moving everything to TF/CPP. Acknowledged. https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2536463002/diff/120001/src/js/promise.js#newc... src/js/promise.js:417: if (PromiseHasUserDefinedRejectHandlerCheck(queue[i], deferred[i])) { On 2016/12/04 17:36:17, gsathya wrote: > On 2016/12/01 16:30:27, jgruber wrote: > > Was this logic refactored in this CL? Is deferred guaranteed to be an array of > > the same length as queue? > > Yes. Yes. They are always updated together in PerformPromiseThen. Acknowledged. https://codereview.chromium.org/2536463002/diff/120001/src/objects-printer.cc File src/objects-printer.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/objects-printer.cc... src/objects-printer.cc:545: void JSPromise::JSPromisePrint(std::ostream& os) { // NOLINT On 2016/12/04 17:36:18, gsathya wrote: > On 2016/12/01 16:30:27, jgruber wrote: > > How much do we gain through JSPromisePrint(promise) over > JSObjectPrint(promise)? > > If the output is similar, we could just go with that. > > Printing out the status as strings could be useful as caitp points out. Acknowledged. https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... File src/runtime/runtime-promise.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:111: if (status->value() == kPromiseFulfilled) { On 2016/12/04 17:36:18, gsathya wrote: > On 2016/12/01 16:30:27, jgruber wrote: > > Maybe more efficient: > > > > tasks((status->value() == kPromiseFulfilled) ? > > promise->fulfill_reactions() : > > promise->reject_reactions(), isolate) > > Hmm, not sure if it's more readable tbh. Changed anyway Acknowledged. https://codereview.chromium.org/2536463002/diff/120001/src/runtime/runtime-pr... src/runtime/runtime-promise.cc:253: if (deferred->IsJSObject()) { On 2016/12/04 17:36:18, gsathya wrote: > On 2016/12/01 16:30:27, jgruber wrote: > > Could you document when deferred is undefined / a JSObject / a FixedArray? The > > comment in object.h does not mention undefined, nor the contents when it is > not > > undefined or a FixedArray. > > Updated comment in objects.h Acknowledged.
https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:389: Node* new_elements = a->AllocateFixedArray(kind, new_capacity, mode); On 2016/12/05 07:58:35, jgruber wrote: > Mentioning this now because I started running into it with RegExp: this probably > needs a check for kMaxRegularHeapObjectSize somewhere. See https://bugs.chromium.org/p/chromium/issues/detail?id=670671
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/29998)
https://codereview.chromium.org/2536463002/diff/240001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2536463002/diff/240001/src/builtins/builtins-... src/builtins/builtins-promise.cc:388: a->StoreFixedArrayElement(new_elements, length, value); This fails on 32bit because `length` never gets untagged, but this uses INTPTR_PARAMETERS by default. Need to change this so that it uses `mode`.
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13711)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
LGTM (rubber-stamp)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by gsathya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jgruber@chromium.org Link to the patchset: https://codereview.chromium.org/2536463002/#ps300001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1480971946525340,
"parent_rev": "d36caad40c24da06cffaed8bf9e764c18602742d", "commit_rev":
"cefd57be5e705a7e4a1f4e2259dd2f6690759078"}
Message was sent while issue was closed.
Description was changed from ========== Object -- New JSObject for promises: JSPromise Builtins -- PromiseThen TFJ -- PromiseCreateAndSet TFJ for internal use -- PerformPromiseThen TFJ for internal use -- PromiseInit for initial promise setup -- SpeciesConstructor for use in PromiseThen -- ThrowIfNotJSReceiver for use in SpeciesConstructor -- AppendPromiseCallback to update FixedArray with new callback -- InternalPerformPromiseThen Promises.js -- Cleanup unused symbols -- Remove PerformPromiseThen -- Remove PromiseThen -- Remove PromiseSet -- Remove PromiseAttachCallbacks Runtime -- PromiseSet to set promise inobject values -- Refactor functions to use FixedArrays for callbacks instead of JSArray -- Runtime_PromiseStatus to return promise status -- Runtime_PromiseResult to return promise result -- Runtime_PromiseDeferred to return deferred attached to promise -- Runtime_PromiseRejectReactions to return reject reactions attached to promise This CL results in a 13.07% improvement in the promises benchmark (over 5 runs). BUG=v8:5343 ========== to ========== Object -- New JSObject for promises: JSPromise Builtins -- PromiseThen TFJ -- PromiseCreateAndSet TFJ for internal use -- PerformPromiseThen TFJ for internal use -- PromiseInit for initial promise setup -- SpeciesConstructor for use in PromiseThen -- ThrowIfNotJSReceiver for use in SpeciesConstructor -- AppendPromiseCallback to update FixedArray with new callback -- InternalPerformPromiseThen Promises.js -- Cleanup unused symbols -- Remove PerformPromiseThen -- Remove PromiseThen -- Remove PromiseSet -- Remove PromiseAttachCallbacks Runtime -- PromiseSet to set promise inobject values -- Refactor functions to use FixedArrays for callbacks instead of JSArray -- Runtime_PromiseStatus to return promise status -- Runtime_PromiseResult to return promise result -- Runtime_PromiseDeferred to return deferred attached to promise -- Runtime_PromiseRejectReactions to return reject reactions attached to promise This CL results in a 13.07% improvement in the promises benchmark (over 5 runs). BUG=v8:5343 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Object -- New JSObject for promises: JSPromise Builtins -- PromiseThen TFJ -- PromiseCreateAndSet TFJ for internal use -- PerformPromiseThen TFJ for internal use -- PromiseInit for initial promise setup -- SpeciesConstructor for use in PromiseThen -- ThrowIfNotJSReceiver for use in SpeciesConstructor -- AppendPromiseCallback to update FixedArray with new callback -- InternalPerformPromiseThen Promises.js -- Cleanup unused symbols -- Remove PerformPromiseThen -- Remove PromiseThen -- Remove PromiseSet -- Remove PromiseAttachCallbacks Runtime -- PromiseSet to set promise inobject values -- Refactor functions to use FixedArrays for callbacks instead of JSArray -- Runtime_PromiseStatus to return promise status -- Runtime_PromiseResult to return promise result -- Runtime_PromiseDeferred to return deferred attached to promise -- Runtime_PromiseRejectReactions to return reject reactions attached to promise This CL results in a 13.07% improvement in the promises benchmark (over 5 runs). BUG=v8:5343 ========== to ========== Object -- New JSObject for promises: JSPromise Builtins -- PromiseThen TFJ -- PromiseCreateAndSet TFJ for internal use -- PerformPromiseThen TFJ for internal use -- PromiseInit for initial promise setup -- SpeciesConstructor for use in PromiseThen -- ThrowIfNotJSReceiver for use in SpeciesConstructor -- AppendPromiseCallback to update FixedArray with new callback -- InternalPerformPromiseThen Promises.js -- Cleanup unused symbols -- Remove PerformPromiseThen -- Remove PromiseThen -- Remove PromiseSet -- Remove PromiseAttachCallbacks Runtime -- PromiseSet to set promise inobject values -- Refactor functions to use FixedArrays for callbacks instead of JSArray -- Runtime_PromiseStatus to return promise status -- Runtime_PromiseResult to return promise result -- Runtime_PromiseDeferred to return deferred attached to promise -- Runtime_PromiseRejectReactions to return reject reactions attached to promise This CL results in a 13.07% improvement in the promises benchmark (over 5 runs). BUG=v8:5343 Committed: https://crrev.com/30b564c76f490f8f6b311a74b25b26cf0a96be2d Cr-Commit-Position: refs/heads/master@{#41503} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/30b564c76f490f8f6b311a74b25b26cf0a96be2d Cr-Commit-Position: refs/heads/master@{#41503}
Message was sent while issue was closed.
https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2536463002/diff/120001/src/builtins/builtins-... src/builtins/builtins-promise.cc:624: a.Branch(a.WordEqual(promise_fun, constructor), &fast_promise_capability, On 2016/12/05 07:58:35, jgruber wrote: > On 2016/12/04 17:36:17, gsathya wrote: > > On 2016/12/01 16:30:26, jgruber wrote: > > > If we want to make this faster at some point, we could do an additional fast > > > path check before 2. above. > > > > Faster how? By skipping the species constructor lookup? > > Actually, thinking about this again, that's probably not a good idea after all > since changing the constructor property does not change the map. Documenting for posterity: gsathya pointed out that changing the constructor property does actually change the map.
Message was sent while issue was closed.
A revert of this CL (patchset #16 id:300001) has been created in https://codereview.chromium.org/2554013002/ by machenbach@chromium.org. The reason for reverting is: Breaks layout tests: https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui... See: https://github.com/v8/v8/wiki/Blink-layout-tests. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
