[webkit-reviews] review granted: [Bug 200898] [JSC] Make Promise implementation faster : [Attachment 377932] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 200898: [JSC] Make Promise implementation faster
https://bugs.webkit.org/show_bug.cgi?id=200898

Attachment 377932: Patch

https://bugs.webkit.org/attachment.cgi?id=377932&action=review




--- Comment #73 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 377932
  --> https://bugs.webkit.org/attachment.cgi?id=377932
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377932&action=review

> Source/JavaScriptCore/ChangeLog:8
> +	   This is the major change of the Promise implementation and it
improves JetStream2/async-fs by 62%.

����

> Source/JavaScriptCore/ChangeLog:10
> +	   1. Make JSPromise C++ friendly

nit: in the future, it'd be nice to split these things in their own patches as
they don't seem like they need to be landed all at once.

> Source/JavaScriptCore/ChangeLog:27
> +	       The old JSPromise constructor was doing very inefficient thing:
JSPromise constructor is InternalFunction in C++, and in it, it

"was doing very inefficient thing" => "was very inefficient"

> Source/JavaScriptCore/ChangeLog:29
> +	       If we can implement JSPromise constructor in all JS code, we can
recognize `executor` and we have a chance to fully inline them.

"in all JS code" => "fully in JS"

> Source/JavaScriptCore/ChangeLog:31
> +	       construct is 100. If we decide inlining it, we should
reinvestigate it further.

"If we decide inlining it, we should reinvestigate it further." => "We might
want to investigate getting it inlined in the future."

Also, might be worth a bug?

> Source/JavaScriptCore/ChangeLog:33
> +	       We can avoid C++ <-> JS dance in such an important operation,
allocating JSPromise. This patch introduces @nakedConstructor

this is cool

> Source/JavaScriptCore/ChangeLog:46
> +	       When converting CreatePromise to NewPromise, we need to get the
correct structure with a specified `callee.prototype`. We mimics the mechanism

mimics => mimic

> Source/JavaScriptCore/ChangeLog:59
> +	       check to take a fast path. And promise reaction now handles 4
cases to handle the case not creating a promise.

the final sentence here doesn't make sense to me

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:-30
> -    const promiseCapability = @newPromiseCapability(@Promise);

you should add Apple copyright to this file too

> Source/JavaScriptCore/builtins/AsyncFromSyncIteratorPrototype.js:42
> +	   var nextDone = !!nextResult.done;
> +	   var nextValue = nextResult.value;

does this change the order we read the fields from the result? Isn't that
observable?

> Source/JavaScriptCore/builtins/PromiseConstructor.js:234
> +    var promise = result;

what's the point of this local variable?

> Source/JavaScriptCore/builtins/PromiseConstructor.js:242
> +    function @resolve(resolution) {
> +	   return @resolvePromiseWithFirstResolvingFunctionCallCheck(promise,
resolution);
> +    }
> +
> +    function @reject(reason) {
> +	   return @rejectPromiseWithFirstResolvingFunctionCallCheck(promise,
reason);
> +    }

why give these private names instead of just "resolve" and "reject"?

> Source/JavaScriptCore/builtins/PromiseConstructor.js:270
> +    function @resolve(resolution) {
> +	   return @resolvePromiseWithFirstResolvingFunctionCallCheck(promise,
resolution);
> +    }
> +
> +    function @reject(reason) {
> +	   return @rejectPromiseWithFirstResolvingFunctionCallCheck(promise,
reason);
> +    }

ditto

> Source/JavaScriptCore/builtins/PromiseOperations.js:134
> +	   return @rejectPromise(promise, @makeTypeError("Resolve a promise
with itself"));

maybe "Cannot resolve a promise with itself"?

> Source/JavaScriptCore/builtins/PromiseOperations.js:146
> +    if (@isPromise(resolution) && then === @then) {

maybe we should name "@then" in the global namespace to be less generic. Maybe
"@defaultPromiseThen" or something?

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:1302
> +	       auto& cacheWriteBarrier = metadata.m_cachedCallee;
> +	       if (!cacheWriteBarrier || cacheWriteBarrier.unvalidatedGet() ==
JSCell::seenMultipleCalleeObjects())
> +		   break;
> +	       JSCell* cachedFunction = cacheWriteBarrier.get();

isn't this a TOCTOU bug? You load the first time and it's ok, and then you load
again on line 1302 and it's now "seenMultipleCalleeObjects". Or is this called
with the mutator paused?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2628
> +		       if
(rareData->allocationProfileWatchpointSet().isStillValid()) {
> +			   if (Structure* structure =
rareData->objectAllocationStructure()) {

why this change? Seems unrelated to your patch

> Source/JavaScriptCore/dfg/DFGUseKind.h:58
> +    PromiseObjectUse,

This seems like it's completely unused. I'd vote for removing it. You said in
your changelog that you would use this for making @isPromise faster, but I
don't see it.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:5923
> +	   ValueFromBlock slowResult = m_out.anchor(vmCall(pointerType(),
m_out.operation(m_node->isInternalPromise() ? operationNewInternalPromise :
operationNewPromise), m_callFrame, weakStructure(m_node->structure())));

why weakStructure here? This feels like a "strong" use case, since all objects
which have this structure might go away, but it shouldn't mean we throw this
code away, since the code might be reachable and we allocate new structures.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6302
> +	   ValueFromBlock promiseStructure =
m_out.anchor(weakStructure(m_graph.registerStructure(m_node->isInternalPromise(
) ? globalObject->internalPromiseStructure() :
globalObject->promiseStructure())));

ditto: doesn't feel like it should be weak

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:528
> +    btpz value, .writeBarrierDone

why would we be putting the JSValue zero? Seems wrong.

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2598
> +    getu(size, OpGetPromiseInternalField, m_index, t2)

this zero extends?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:2607
> +    getu(size, OpPutPromiseInternalField, m_index, t1)

this zero extends?

> Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:289
> +    if (constructorAsObject->type() == JSFunctionType &&
jsCast<JSFunction*>(constructorAsObject)->canUseAllocationProfile()) {

nit: I think it's nicer to jsDynamicCast<JSFunction*> here instead of checking
type manually.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1684
> +    visitor.append(thisObject->m_internalPromiseConstructor);

oops

> Source/JavaScriptCore/runtime/JSPromise.h:72
> +    static ptrdiff_t offsetOfInternalField(unsigned index) { return
OBJECT_OFFSETOF(JSPromise, m_internalFields) + index *
sizeof(WriteBarrier<Unknown>); }

does std::array give us this guarantee?