Bug 193031 – [JSC] Optimize Object.prototype.toString

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Optimize Object.prototype.toString

[Yusuke Suzuki]

Created attachment 358056 [details]
Patch

[Yusuke Suzuki]

Created attachment 358057 [details]
Patch

[Yusuke Suzuki]

Created attachment 358058 [details]
Patch

[Yusuke Suzuki]

Created attachment 358060 [details]
Patch

[EWS Watchlist]

Comment on attachment 358060 [details]
Patch

Attachment 358060 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10541563

New failing tests:
media/modern-media-controls/airplay-support/airplay-support-disable-event-listeners-until-play.html

[EWS Watchlist]

Created attachment 358062 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Yusuke Suzuki]

Created attachment 358078 [details]
Patch

[EWS Watchlist]

Comment on attachment 358078 [details]
Patch

Attachment 358078 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10551775

New failing tests:
media/modern-media-controls/airplay-support/airplay-support-disable-event-listeners-until-play.html

[EWS Watchlist]

Created attachment 358080 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Yusuke Suzuki]

Created attachment 358110 [details]
Patch

[Yusuke Suzuki]

Created attachment 358111 [details]
Patch

[Keith Miller]

Comment on attachment 358111 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        It is called so many times in wtb-lebab.js. This patch optimizes

Did you mean to say more in this sentence? If not, I would get rid of the "so" and make this: "It is called many times in wtb-lebab.js, for instance"

> Source/JavaScriptCore/ChangeLog:12
> +        1. We should emit code looking up cached one in DFG and FTL.

Nit: cached one => a cached result.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12473
> +        auto isUndefined = m_jit.branchIfUndefined(sourceRegs);
> +        m_jit.move(TrustedImmPtr::weakPointer(m_jit.graph(), m_jit.vm()->smallStrings.nullObjectString()), resultGPR);
> +        auto done = m_jit.jump();
> +        isUndefined.link(&m_jit);
> +        m_jit.move(TrustedImmPtr::weakPointer(m_jit.graph(), m_jit.vm()->smallStrings.undefinedObjectString()), resultGPR);
> +        done.link(&m_jit);

Don't you need a case here for booleans? If so, can you add a test?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6449
> +            LBasicBlock undefinedCase = m_out.newBlock();
> +            LBasicBlock nullCase = m_out.newBlock();
> +            LBasicBlock continuation = m_out.newBlock();
> +
> +            speculate(m_node->child1());
> +            LValue source = lowJSValue(m_node->child1(), ManualOperandSpeculation);
> +            LValue valueIsUndefined = m_out.equal(source, m_out.constInt64(ValueUndefined));
> +            m_out.branch(valueIsUndefined, unsure(undefinedCase), unsure(nullCase));
> +
> +            LBasicBlock lastNext = m_out.appendTo(undefinedCase, nullCase);
> +            ValueFromBlock undefinedString = m_out.anchor(weakPointer(vm().smallStrings.undefinedObjectString()));
> +            m_out.jump(continuation);
> +
> +            m_out.appendTo(nullCase, continuation);
> +            ValueFromBlock nullString = m_out.anchor(weakPointer(vm().smallStrings.nullObjectString()));
> +            m_out.jump(continuation);
> +
> +            m_out.appendTo(continuation, lastNext);
> +            setJSValue(m_out.phi(pointerType(), undefinedString, nullString));
> +            return;

Ditto on the boolean here.

[Yusuke Suzuki]

Comment on attachment 358111 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        It is called so many times in wtb-lebab.js. This patch optimizes
> 
> Did you mean to say more in this sentence? If not, I would get rid of the "so" and make this: "It is called many times in wtb-lebab.js, for instance"

Yeah, fixed by dropping `so`.

>> Source/JavaScriptCore/ChangeLog:12
>> +        1. We should emit code looking up cached one in DFG and FTL.
> 
> Nit: cached one => a cached result.

Nice. Fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12473
>> +        done.link(&m_jit);
> 
> Don't you need a case here for booleans? If so, can you add a test?

Oops, fixed to use OtherUse and SpecOther. Misc is not right.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6449
>> +            return;
> 
> Ditto on the boolean here.

Fixed.

[Yusuke Suzuki]

Created attachment 358166 [details]
Patch

[Yusuke Suzuki]

Created attachment 358211 [details]
Patch

[Yusuke Suzuki]

Created attachment 358214 [details]
Patch

[Yusuke Suzuki]

Ping?

[Saam Barati]

Comment on attachment 358214 [details]
Patch

r=me

[Saam Barati]

Comment on attachment 358214 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        1. We should emit code looking up cached one in DFG and FTL.

cached one => cached to string

[Yusuke Suzuki]

Comment on attachment 358214 [details]
Patch

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

Thank you!

>> Source/JavaScriptCore/ChangeLog:12
>> +        1. We should emit code looking up cached one in DFG and FTL.
> 
> cached one => cached to string

Oops, fixed.

[Yusuke Suzuki]

Committed r239612: <https://trac.webkit.org/changeset/239612>

[Radar WebKit Bug Importer]

<rdar://problem/47049168>