Bug 148118 - Introduce non-user-observable Promise functions to use Promises internally
Summary: Introduce non-user-observable Promise functions to use Promises internally
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks: 148136
  Show dependency treegraph
 
Reported: 2015-08-17 20:17 PDT by Yusuke Suzuki
Modified: 2015-08-18 15:32 PDT (History)
1 user (show)

See Also:


Attachments
Patch (25.09 KB, patch)
2015-08-17 20:35 PDT, Yusuke Suzuki
saam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-08-17 20:17:04 PDT
Spawned from https://bugs.webkit.org/show_bug.cgi?id=147876
Comment 1 Yusuke Suzuki 2015-08-17 20:35:00 PDT
Created attachment 259230 [details]
Patch
Comment 2 Saam Barati 2015-08-18 11:18:58 PDT
Comment on attachment 259230 [details]
Patch

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

r=me with suggestions.

> Source/JavaScriptCore/builtins/PromiseConstructor.js:158
> +    var remainingElementsCount = 1;

I think this would be easier to read if we started this at 0 and then check if the array length is 0 explicitly.
Below, we can do:
If (array length is zero): "promiseCapability.@resolve.@call(undefined, values);"
else: for (....) { }

(or switch the if/else)

> Source/JavaScriptCore/runtime/JSPromiseDeferred.cpp:75
> +static void callFunction(ExecState* exec, JSValue function, JSValue value)

This seems like it'd get inlined but maybe you should explicitly mark it as inline.
Comment 3 Yusuke Suzuki 2015-08-18 11:32:58 PDT
Comment on attachment 259230 [details]
Patch

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

Thank you for your review, Saam!

>> Source/JavaScriptCore/builtins/PromiseConstructor.js:158
>> +    var remainingElementsCount = 1;
> 
> I think this would be easier to read if we started this at 0 and then check if the array length is 0 explicitly.
> Below, we can do:
> If (array length is zero): "promiseCapability.@resolve.@call(undefined, values);"
> else: for (....) { }
> 
> (or switch the if/else)

That's great idea!

Originally, this implementation corresponds to the spec's Promise.all implementation.
In Promise.all implementation, since we need to iterate the taken iterable parameter (this may not be an array, so we cannot perform iterable.length) by for-of (this is user-observable), we and the spec need to perform the current code.
But this privateAll does not have such a limitation; All access should not be user-observable. So we can just do so; checking the array.length === 0 before iterating the given value!

>> Source/JavaScriptCore/runtime/JSPromiseDeferred.cpp:75
>> +static void callFunction(ExecState* exec, JSValue function, JSValue value)
> 
> This seems like it'd get inlined but maybe you should explicitly mark it as inline.

Sounds nice. I'll add `inline` to it.
Comment 4 Yusuke Suzuki 2015-08-18 12:38:36 PDT
After seeing the code, I've still found the problem; trapping the operation from the user side.
To avoid the problem, I'll

+ land this patch without @then. The problem of then will be solved in the different patch
+ create the new constructor/prototype instance for the internal promises https://bugs.webkit.org/show_bug.cgi?id=148136

So, at first, I'll land this patch without @then.
Comment 5 Yusuke Suzuki 2015-08-18 13:44:02 PDT
(In reply to comment #4)
> After seeing the code, I've still found the problem; trapping the operation
> from the user side.
> To avoid the problem, I'll
> 
> + land this patch without @then. The problem of then will be solved in the
> different patch
> + create the new constructor/prototype instance for the internal promises
> https://bugs.webkit.org/show_bug.cgi?id=148136
> 
> So, at first, I'll land this patch without @then.

I discussed with Saam offline and we decide to do so.
Comment 6 Yusuke Suzuki 2015-08-18 15:32:18 PDT
Committed r188603: <http://trac.webkit.org/changeset/188603>