Bug 144093 - Implement @isConstructor bytecode intrinsic and bytecode for that
Summary: Implement @isConstructor bytecode intrinsic and bytecode for that
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Minor
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
: 193057 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-04-23 02:41 PDT by Yusuke Suzuki
Modified: 2020-06-12 19:56 PDT (History)
14 users (show)

See Also:


Attachments
Patch (50.45 KB, patch)
2020-05-11 09:17 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (189.20 KB, patch)
2020-05-12 13:25 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (212.61 KB, patch)
2020-05-12 17:51 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (312.31 KB, patch)
2020-05-12 20:27 PDT, Alexey Shvayka
no flags 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-04-23 02:41:07 PDT
Currently, `typeof XXX === "function"` is used (in Array.from)
However, it's not accuate to the spec. According to the spec, this should be queried by `isConstructor` abstract operation.
While `isConstructor` queries the given object has `[[Construct]]`, `typeof thisObj === "function"` queries the given object has `[[Call]]`.

To fix that, we introduce a new bytecode op_is_constructor and use it with bytecode intrinsic @isConstructor.
Comment 2 Alexey Shvayka 2020-05-11 09:17:18 PDT
Created attachment 399023 [details]
Patch
Comment 3 Alexey Shvayka 2020-05-11 09:17:46 PDT
(In reply to Alexey Shvayka from comment #2)
> Created attachment 399023 [details]
> Patch

With warm-up runs, --outer 16:

                               r261438                    patch                                       

array-from-arraylike       86.0394+-2.1520     ^     53.7155+-1.7785        ^ definitely 1.6018x faster
array-of                   76.1386+-3.3354     ^     45.5391+-1.6705        ^ definitely 1.6719x faster
is-constructor             53.2389+-1.4687     ^     38.8312+-1.3457        ^ definitely 1.3710x faster

<geometric>                70.3023+-1.1954     ^     45.5675+-0.8233        ^ definitely 1.5428x faster
Comment 4 Keith Miller 2020-05-11 09:19:54 PDT
(In reply to Alexey Shvayka from comment #3)
> (In reply to Alexey Shvayka from comment #2)
> > Created attachment 399023 [details]
> > Patch
> 
> With warm-up runs, --outer 16:
> 
>                                r261438                    patch             
> 
> 
> array-from-arraylike       86.0394+-2.1520     ^     53.7155+-1.7785       
> ^ definitely 1.6018x faster
> array-of                   76.1386+-3.3354     ^     45.5391+-1.6705       
> ^ definitely 1.6719x faster
> is-constructor             53.2389+-1.4687     ^     38.8312+-1.3457       
> ^ definitely 1.3710x faster
> 
> <geometric>                70.3023+-1.1954     ^     45.5675+-0.8233       
> ^ definitely 1.5428x faster

Nice!
Comment 5 Keith Miller 2020-05-11 09:47:09 PDT
Comment on attachment 399023 [details]
Patch

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

r=me but what fixed the test262 test? It's not obvious from the patch.

> Source/JavaScriptCore/ChangeLog:9
> +        This change replaces @isConstructor link-time-constant with bytecode intrinsic and utilizes it
> +        in ClassExprNode::emitBytecode() according to the spec [1], aligning JSC with V8 and SpiderMonkey.

Can you elaborate on what semantically changed here? From the rest of the ChangeLog this seems like a perf-only change.

> Source/JavaScriptCore/builtins/ArrayConstructor.js:31
> +    var array = this !== @Array && @isConstructor(this) ? new this(length) : @newArrayWithSize(length);

Is this just for perf?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1737
> +            if (!(child.m_type & (SpecFunction | SpecProxyObject))) {

Can you add a FIXME + bugzilla to someday iterate all Structures in the abstract values structure set and see if we know they are all constructible? Right now that's not possible because we don't have the object but for many (most?) structures we shouldn't need the object itself to answer this question.
Comment 6 Yusuke Suzuki 2020-05-11 12:05:38 PDT
Can you ensure that total number of bytecodes does not exceed 255 yet?
Comment 7 Alexey Shvayka 2020-05-11 14:31:09 PDT
*** Bug 193057 has been marked as a duplicate of this bug. ***
Comment 8 Alexey Shvayka 2020-05-12 13:25:12 PDT
Created attachment 399164 [details]
Patch

Adjust WPT expectations, defer changes unrelated to test262 fix, make ChangeLog more clear, add FIXME.
Comment 9 Alexey Shvayka 2020-05-12 13:31:39 PDT
(In reply to Yusuke Suzuki from comment #6)
> Can you ensure that total number of bytecodes does not exceed 255 yet?

As of r261556, we have 185 ops in Bytecode section of bytecode/BytecodeList.rb, 67 of which are in groups. Am I counting right?
Comment 10 Alexey Shvayka 2020-05-12 17:51:32 PDT
Created attachment 399212 [details]
Patch

Update mac-wk1 expectations because the builder has ENABLE_DATALIST_ELEMENT off.
Comment 11 Alexey Shvayka 2020-05-12 20:27:31 PDT
Created attachment 399234 [details]
Patch

Rework expectations for wpt/custom-elements/builtin-coverage.html.
Comment 12 EWS 2020-05-12 23:41:38 PDT
Committed r261600: <https://trac.webkit.org/changeset/261600>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399234 [details].
Comment 13 Radar WebKit Bug Importer 2020-05-12 23:42:17 PDT
<rdar://problem/63171871>
Comment 14 Chris Dumez 2020-05-27 14:39:28 PDT
I suspect this may have caused Bug 212430.
Comment 15 Brent Fulgham 2020-06-12 14:45:57 PDT
Note:

This change was rolled out due to web page loading problems:

Committed r262231: <https://trac.webkit.org/changeset/262231>
Comment 16 Chris Dumez 2020-06-12 14:58:57 PDT
(In reply to Brent Fulgham from comment #15)
> Note:
> 
> This change was rolled out due to web page loading problems:
> 
> Committed r262231: <https://trac.webkit.org/changeset/262231>

My understanding is that this was not a revert but rather a follow-up fix.
Comment 17 Yusuke Suzuki 2020-06-12 19:56:35 PDT
(In reply to Chris Dumez from comment #16)
> (In reply to Brent Fulgham from comment #15)
> > Note:
> > 
> > This change was rolled out due to web page loading problems:
> > 
> > Committed r262231: <https://trac.webkit.org/changeset/262231>
> 
> My understanding is that this was not a revert but rather a follow-up fix.

Right. I fixed the incorrect DFG AI rule included in this patch. The other part remains.