Bug 144093 – Implement @isConstructor bytecode intrinsic and bytecode for that

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

ES6 Class is also require it.
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-runtime-semantics-classdefinitionevaluation

[Alexey Shvayka]

Created attachment 399023 [details]
Patch

[Alexey Shvayka]

(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

[Keith Miller]

(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!

[Keith Miller]

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.

[Yusuke Suzuki]

Can you ensure that total number of bytecodes does not exceed 255 yet?

[Alexey Shvayka]

*** Bug 193057 has been marked as a duplicate of this bug. ***

[Alexey Shvayka]

Created attachment 399164 [details]
Patch

Adjust WPT expectations, defer changes unrelated to test262 fix, make ChangeLog more clear, add FIXME.

[Alexey Shvayka]

(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?

[Alexey Shvayka]

Created attachment 399212 [details]
Patch

Update mac-wk1 expectations because the builder has ENABLE_DATALIST_ELEMENT off.

[Alexey Shvayka]

Created attachment 399234 [details]
Patch

Rework expectations for wpt/custom-elements/builtin-coverage.html.

[EWS]

Committed r261600: <https://trac.webkit.org/changeset/261600>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399234 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63171871>

[Chris Dumez]

I suspect this may have caused Bug 212430.

[Brent Fulgham]

Note:

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

Committed r262231: <https://trac.webkit.org/changeset/262231>

[Chris Dumez]

(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.

[Yusuke Suzuki]

(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.