Bug 205147 - DFG and FTL expects String.prototype to not qualify for StringObjectUse.
Summary: DFG and FTL expects String.prototype to not qualify for StringObjectUse.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-12-11 17:21 PST by Mark Lam
Modified: 2019-12-12 10:09 PST (History)
6 users (show)

See Also:


Attachments
proposed patch. (5.66 KB, patch)
2019-12-11 17:35 PST, Mark Lam
saam: review+
Details | Formatted Diff | Diff
patch for landing. (5.67 KB, patch)
2019-12-11 17:44 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2019-12-11 17:21:48 PST
Currently, String.prototype's JSType is StringObjectType.

However, in the compiler, there are a few places that expect that the String.prototype value to not qualify as StringObjectUse.  These places are:
1. SpeculatedType.cpp's speculationFromClassInfo() will speculate SpecObjectOther for the StringPrototype object.
2. DFGFixupPhase.cpp's addCheckStructureForOriginalStringObjectUse() only does a CheckStructure against globalObject->stringObjectStructure().  It does not check against String.prototype's structure.

To resolve this discrepancy, we can either do:
a. change String.prototype's JSType to something else.
b. fix the places in the compiler to accept String.prototype as StringObjectUse.

(a) is trivial and cheap to do.  (b) is doable but will result in less optimal compiled code.  Since passing String.prototype as a StringObject is expected to be a rare thing in JS code, it's not worth incurring the cost for (b).  We'll apply (a).

<rdar://problem/57748888>
Comment 1 Mark Lam 2019-12-11 17:35:16 PST
Created attachment 385465 [details]
proposed patch.
Comment 2 Saam Barati 2019-12-11 17:37:59 PST
Comment on attachment 385465 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:3
> +        FTL expects String.prototype to not qualify for StringObjectUse.

it's not just the FTL. It's DFG+FTL

> Source/JavaScriptCore/runtime/JSType.h:120
> +    // Start StringObjectType types.

this seems unnecessary

> Source/JavaScriptCore/runtime/JSType.h:123
> +    // End StringObjectType types.

ditto

No need to call something out we don't rely on IMO
Comment 3 Saam Barati 2019-12-11 17:39:28 PST
Comment on attachment 385465 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/JSType.h:123
>> +    // End StringObjectType types.
> 
> ditto
> 
> No need to call something out we don't rely on IMO

ignore me
Comment 4 Mark Lam 2019-12-11 17:44:20 PST
Created attachment 385466 [details]
patch for landing.

Thanks for the review.
Comment 5 Mark Lam 2019-12-12 10:09:21 PST
The Win EWS bot failures are just due to flakiness.  A re-run of the EWS bot produced different failures, and the failure cannot be due to this change.

Landed in r253432: <http://trac.webkit.org/r253432>.