Bug 205544 – Replace uses of Box<Identifier> with a new CacheableIdentifier class.

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

[Mark Lam]

The CacheableIdentifier is effectively a tagged union of a JSCell* or an Identifier.  The JSCell* in this case can be either a Symbol* or a JSString* that is backed by an atom string.  The VM runtime ensures that we'll never try to cache an identifier from a JSCell that is not one of these.  With this CacheableIdentifier, we no longer need to worry about avoiding a ref/deref of the underlying UniquedStringImpl from the compiler or GC thread.  Instead, we need to visit CacheableIdentifiers during GC scans to keep the JSCell in it alive, and that JSCell will, in turn, keep the underlying UniquedStringImpl alive.

<rdar://problem/58041800>

[Mark Lam]

Created attachment 386308 [details]
work in progress for EWS testing.

[Mark Lam]

Created attachment 386318 [details]
proposed patch.

[Mark Lam]

Comment on attachment 386318 [details]
proposed patch.

Taking out of review while I research a few more things.

[Mark Lam]

Comment on attachment 386318 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:36
> +           StructureStubInfo::propagateTransitions() is inappropriate for this because
> +           it is only called after GC has finished.  As a result, it won't keep
> +           the JSCell identifiers alive.

OK, I'm pretty sure this statement is wrong.  I see that propagateTransitions() is called below Heap::runFixpointPhase().  However, empirically, after adding an AccessCase to the StructureStubInfo and barriering it, I see CodeBlock::visitChildren() called for the relevant codeBlock in the current GC cycle.  However, CodeBlock::propagateTransitions() for this codeBlock doesn't get called until the next GC cycle.  As a result, the JSCell identifiers in CacheableIdentifiers get swept before the next scan.  While the current solution of explicitly calling StructureStubInfo::visitChildren() via CodeBlock::visitChildren() works, I need to research why propagateTransition() doesn't get called till the next GC cycle.

[Saam Barati]

Did you forget a write barrier?

[Mark Lam]

(In reply to Saam Barati from comment #5)
> Did you forget a write barrier?

StructureStubInfo::addAccessCase() takes care of the writeBarrier.  It is also meaningless to do the barrier before the AccessCase is added to the StructureStubInfo.

In my test, I also see CodeBlock::visitChildren() called after the barrier in StructureStubInfo::addAccessCase().  However, that doesn't necessarily lead to CodeBlock::propagateTransitions() being called.

[Mark Lam]

OK, I've found at least one example of why we can't rely on propagateTransition() to visit the CachedIdentifier.  Here's a scenario I've recorded in time order:

1. StructureStubInfo 0x107140110 addAccessCase 0x1071d5e70 with a JSString identifier 0x1075fedd0.

2. StructureStubInfo 0x107140110 writeBarriers the owner codeBlock 0x1075bbac0.

3. CodeBlock 0x1075bbac0 gets optimized to DFG codeBlock 0x1075bb480.
   Optimized get#CFJvFA:[0x1075bb480->0x1075bbac0->0x1075db100, NoneFunctionCall, 14] using DFGMode with DFG into 1344 bytes in 8.964644 ms.

4. GC calls CodeBlock::visitChildren() on the barriered codeBlock 0x1075bbac0.

5. CodeBlock 0x1075bbac0 calls stronglyVisitStrongReferences() which appends m_ownerExecutable 0x1075db100.

6. GC calls FunctionExecutable::visitChildren() on ownerExecutable 0x1075db100, and appends m_codeBlockForCall 0x1075a4000 (a ExecutableToCodeBlockEdge).

7. GC calls ExecutableToCodeBlockEdge::visitChildren() on edge 0x1075a4000, which calls ExecutableToCodeBlockEdge::runConstraint(), which in turn calls codeBlock->propagateTransitions() on codeBlock 0x1075bb480.

Note: codeBlock 0x1075bb480 is the DFG codeBlock, not the baseline codeBlock that owns the StructureStubInfo.

When the StructureStubInfo barriers the owner CodeBlock, we can guarantee that its visitChildren() gets called, but there doesn't appear to be a guarantee that the owner CodeBlock's propagateTransitions() will get called.

Since the cell identifier in CachedIdentifiers are supposed to be strong references, we should just call a visitChildren() on the codeBlock's StructureStubInfos (which is what this patch does).  I'll update the ChangeLog with this new info.

[Mark Lam]

(In reply to Mark Lam from comment #7)
> Since the cell identifier in CachedIdentifiers are supposed to be strong
> references, we should just call a visitChildren() on the codeBlock's
> StructureStubInfos (which is what this patch does).  I'll update the
> ChangeLog with this new info.

By "CachedIdentifiers", I meant CacheableIdentifiers.

[Mark Lam]

Created attachment 386325 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 386325 [details]
proposed patch.

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

Can you ensure that we are emitting write-barrier when storing CacheableIdentifier?
Another question: If identifier is used by JIT code, do we keep cells live when (1) AccessCases are destroyed, while (2) JIT code is live on the stack? Do we need marking them in GCAwareJITStubRoutine?

> Source/JavaScriptCore/ChangeLog:31
> +        1. Add a visitChildren() method to StructureStubInfo, PolymorphicAccess, and

Use `visitAggregate` name instead of `visitChildren`. The difference is,

1. visitChildren is defined in JSCell. And it is a function of owner cell.
2. visitAggregate is defined in non JSCell, and owner of visited cells are not this struct: cells are owned by CodeBlock cell, not by StructureStubInfo.

> Source/JavaScriptCore/ChangeLog:47
> +        2. Also add visitChildren() to ModuleNamespaceData.

Ditto

> Source/JavaScriptCore/runtime/CacheableIdentifier.cpp:34
> +JS_EXPORT_PRIVATE void CacheableIdentifier::dump(PrintStream& out) const

JS_EXPORT_PRIVATE in cpp is not necessary.

> Source/JavaScriptCore/runtime/CacheableIdentifier.h:79
> +    inline void visitChildren(SlotVisitor&) const;

Ditto.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #10)
> Comment on attachment 386325 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386325&action=review
> 
> Can you ensure that we are emitting write-barrier when storing
> CacheableIdentifier?
> Another question: If identifier is used by JIT code, do we keep cells live
> when (1) AccessCases are destroyed, while (2) JIT code is live on the stack?
> Do we need marking them in GCAwareJITStubRoutine?
In this case, won't the CodeBlock also be on the stack?

> 
> > Source/JavaScriptCore/ChangeLog:31
> > +        1. Add a visitChildren() method to StructureStubInfo, PolymorphicAccess, and
> 
> Use `visitAggregate` name instead of `visitChildren`. The difference is,
> 
> 1. visitChildren is defined in JSCell. And it is a function of owner cell.
> 2. visitAggregate is defined in non JSCell, and owner of visited cells are
> not this struct: cells are owned by CodeBlock cell, not by StructureStubInfo.
> 
> > Source/JavaScriptCore/ChangeLog:47
> > +        2. Also add visitChildren() to ModuleNamespaceData.
> 
> Ditto
> 
> > Source/JavaScriptCore/runtime/CacheableIdentifier.cpp:34
> > +JS_EXPORT_PRIVATE void CacheableIdentifier::dump(PrintStream& out) const
> 
> JS_EXPORT_PRIVATE in cpp is not necessary.
> 
> > Source/JavaScriptCore/runtime/CacheableIdentifier.h:79
> > +    inline void visitChildren(SlotVisitor&) const;
> 
> Ditto.

[Saam Barati]

Comment on attachment 386325 [details]
proposed patch.

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

> Source/JavaScriptCore/bytecode/AccessCase.cpp:62
> +    , m_identifier(identifier)

you need a write barrier after this, right?

> Source/JavaScriptCore/bytecode/AccessCase.cpp:138
> +        ASSERT(!identifier.isCell());

do these assertions really matter?

> Source/JavaScriptCore/bytecode/GetByStatus.cpp:297
> +                GetByIdVariant variant(trackIdentifiers == TrackIdentifiers::Yes ? access.identifier() : nullptr, StructureSet(structure), complexGetStatus.offset(),

can we just get rid of this TrackIdentifiers stuff now?

> Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:70
> +    m_getByIdSelfIdentifier = identifier;

do we still need this?

If so, I believe you need a write barrier here.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:5643
> +                        addToGraph(CheckCell, OpInfo(frozen), property);

why? CheckIdent is better for strings. I think you should CheckCell for Symbol, since they're hash-consed.  And for string, you should CheckIdent.

You may have two JSString* with the same impl

> Source/JavaScriptCore/runtime/CacheableIdentifierInlines.h:76
> +    if (string->isRope())
> +        return false;
> +    return string->getValueImpl()->isAtom();

nit: The better abstraction is to call tryGetValueImpl() here and return false if it returns nullptr.

> Source/JavaScriptCore/runtime/CacheableIdentifierInlines.h:116
> +    return uid() == other.uid();

👍🏼

[Saam Barati]

(In reply to Saam Barati from comment #11)
> (In reply to Yusuke Suzuki from comment #10)
> > Comment on attachment 386325 [details]
> > proposed patch.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=386325&action=review
> > 
> > Can you ensure that we are emitting write-barrier when storing
> > CacheableIdentifier?
> > Another question: If identifier is used by JIT code, do we keep cells live
> > when (1) AccessCases are destroyed, while (2) JIT code is live on the stack?
> > Do we need marking them in GCAwareJITStubRoutine?
> In this case, won't the CodeBlock also be on the stack?
Yeah, my guess is we don't need to mark these as part of GCAwareJITStubRoutine since they're a normal GC reference (e.g, we'll always mark it), not a weak reference, like the other references inside AccessCase.

> 
> > 
> > > Source/JavaScriptCore/ChangeLog:31
> > > +        1. Add a visitChildren() method to StructureStubInfo, PolymorphicAccess, and
> > 
> > Use `visitAggregate` name instead of `visitChildren`. The difference is,
> > 
> > 1. visitChildren is defined in JSCell. And it is a function of owner cell.
> > 2. visitAggregate is defined in non JSCell, and owner of visited cells are
> > not this struct: cells are owned by CodeBlock cell, not by StructureStubInfo.
> > 
> > > Source/JavaScriptCore/ChangeLog:47
> > > +        2. Also add visitChildren() to ModuleNamespaceData.
> > 
> > Ditto
> > 
> > > Source/JavaScriptCore/runtime/CacheableIdentifier.cpp:34
> > > +JS_EXPORT_PRIVATE void CacheableIdentifier::dump(PrintStream& out) const
> > 
> > JS_EXPORT_PRIVATE in cpp is not necessary.
> > 
> > > Source/JavaScriptCore/runtime/CacheableIdentifier.h:79
> > > +    inline void visitChildren(SlotVisitor&) const;
> > 
> > Ditto.

[Mark Lam]

Comment on attachment 386325 [details]
proposed patch.

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

>>>> Source/JavaScriptCore/ChangeLog:31
>>>> +        1. Add a visitChildren() method to StructureStubInfo, PolymorphicAccess, and
>>> 
>>> Use `visitAggregate` name instead of `visitChildren`. The difference is,
>>> 
>>> 1. visitChildren is defined in JSCell. And it is a function of owner cell.
>>> 2. visitAggregate is defined in non JSCell, and owner of visited cells are not this struct: cells are owned by CodeBlock cell, not by StructureStubInfo.
>> 
>> 
> 
> 

I've renamed visitChildren() to visitAggregate().

>> Source/JavaScriptCore/ChangeLog:47
>> +        2. Also add visitChildren() to ModuleNamespaceData.
> 
> Ditto

Fixed.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:62
>> +    , m_identifier(identifier)
> 
> you need a write barrier after this, right?

Nope.  It's meaningless to write barrier the owner CodeBlock here when this AccessCase has not yet been added to a StructureStubInfo that is reachable from the owner CodeBlock in a GC scan.  Doing so will at most cause the owner CodeBlock to be scanned by the GC, but it will not cause this AccessCase to be scanned by the GC.  StructureStubInfo::addAccessCase() already takes care of issuing the write barrier on the owner CodeBlock after the AccessCase has been added.  That is the right place for the write barrier.

>> Source/JavaScriptCore/bytecode/AccessCase.cpp:138
>> +        ASSERT(!identifier.isCell());
> 
> do these assertions really matter?

It's just documentation that we should never expect to see the isCell() case on this path.

>> Source/JavaScriptCore/bytecode/GetByStatus.cpp:297
>> +                GetByIdVariant variant(trackIdentifiers == TrackIdentifiers::Yes ? access.identifier() : nullptr, StructureSet(structure), complexGetStatus.offset(),
> 
> can we just get rid of this TrackIdentifiers stuff now?

I see that this TrackIdentifiers was added in r252763.  It isn't totally clear to me that this wasn't for a reason other than just to avoid keeping the Box<Identifier> alive for GetById cases.  GetByIdVariant::overlaps() does check for the identifier being null, and we are forcing the identifier to be null here for GetById cases.

However, from reading through the code for GetByIdVariant::attemptToMerge() and the ChangeLog from r252763, I think it's safe to merge a GetById variant with a GetByVal variant if the GetByVal has the same identifier and offset.  Hence, there's no reason to force the identifier to null for the GetById case.  I'll go ahead and remove all the TrackIdentifiers.

>> Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:70
>> +    m_getByIdSelfIdentifier = identifier;
> 
> do we still need this?
> 
> If so, I believe you need a write barrier here.

It looks like the reason for capturing this identifier is entirely so that in computeForStubInfoWithoutExitSiteFeedback(), we can retrieve the identifier so that the GetByIdVariant can do its overlaps() test correctly.  I'm not entirely sure but from my naive reading of the code, I think we still need this.  I've added a write barrier here.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:5643
>> +                        addToGraph(CheckCell, OpInfo(frozen), property);
> 
> why? CheckIdent is better for strings. I think you should CheckCell for Symbol, since they're hash-consed.  And for string, you should CheckIdent.
> 
> You may have two JSString* with the same impl

Good point.  Fixed to do CheckIdent for strings, and CheckCell for Symbol.  I still freeze the string if isCell().

>> Source/JavaScriptCore/runtime/CacheableIdentifier.cpp:34
>> +JS_EXPORT_PRIVATE void CacheableIdentifier::dump(PrintStream& out) const
> 
> JS_EXPORT_PRIVATE in cpp is not necessary.

Fixed.

>> Source/JavaScriptCore/runtime/CacheableIdentifier.h:79
>> +    inline void visitChildren(SlotVisitor&) const;
> 
> Ditto.

Fixed.

>> Source/JavaScriptCore/runtime/CacheableIdentifierInlines.h:76
>> +    return string->getValueImpl()->isAtom();
> 
> nit: The better abstraction is to call tryGetValueImpl() here and return false if it returns nullptr.

Fixed.

[Mark Lam]

Created attachment 386616 [details]
proposed patch.

Uploading new patch with all the feedback applied for EWS testing and for commenting.  Still need to think through Yusuke's questions about potential GC correctness issues.

[Saam Barati]

Comment on attachment 386616 [details]
proposed patch.

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

> Source/JavaScriptCore/jit/Repatch.cpp:203
> +            // Deliberately use the Identifier (instead of JSCell*) form since
> +            // we have an immortal Identifier in this case.
> +            propertyName = vm.propertyNames->length;

Yeah, this works. But it only works for something that really is immortal, since CacheableIdentifier isn't a +1 ref. So it's slightly worrisome to do IMO since it might encourage a pattern which isn't always safe.

[Mark Lam]

Comment on attachment 386616 [details]
proposed patch.

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

>> Source/JavaScriptCore/jit/Repatch.cpp:203
>> +            propertyName = vm.propertyNames->length;
> 
> Yeah, this works. But it only works for something that really is immortal, since CacheableIdentifier isn't a +1 ref. So it's slightly worrisome to do IMO since it might encourage a pattern which isn't always safe.

I thought this was needed because StructureStubInfo doesn't have a PolymorphicAccess stub for CacheType::ArrayLength and CacheType::StringLength and hence, is unable to scan the cell in the AccessCase identifier.  However, it looks like I misread the code.  For both CacheType::ArrayLength and CacheType::StringLength, we don't add any AccessCases to the StructureStubInfo.  Hence, there is no AccessCase to scan.  I'll remove this change.

[Mark Lam]

(In reply to Mark Lam from comment #15)
> Created attachment 386616 [details]
> proposed patch.
> 
> Uploading new patch with all the feedback applied for EWS testing and for
> commenting.  Still need to think through Yusuke's questions about potential
> GC correctness issues.

I've considered Yusuke's GC questions and found and fixed some bugs in the patch.  Will upload a new shortly with more ChangeLog comments addressing Yusuke's concerns.

[Mark Lam]

Created attachment 387410 [details]
proposed patch.

[Saam Barati]

Comment on attachment 387410 [details]
proposed patch.

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

r=me

Looks really good overall. Just two main comments where things are iffy

> Source/JavaScriptCore/ChangeLog:9
> +        The introduction of the use of Box<Identifier> is to get around having to

"is to get" => "was to get"

> Source/JavaScriptCore/ChangeLog:16
> +        This patch fixes this by replacing uses of Box<Identifier> with a

"with a" => "with"

> Source/JavaScriptCore/ChangeLog:79
> +        The GCSafeConcurrentJSLocker ensures that the CacheableIdentifier is will be

"is will be" => will be"

That's just a lock. How does it ensure it'll be protected from GC? (I know the answer, that stronglyVisitStrongReferences holds the lock, but this sentence is slightly confusing because it doesn't indicate that.)

> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:2
> + * Copyright (C) 2017-2019 Apple Inc. All rights reserved.

I think these should be 2020 (you have a few 2019)

> Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:71
> +    m_getByIdSelfIdentifier = identifier;
> +    codeBlock->vm().heap.writeBarrier(codeBlock);

I wonder if somehow the compiler could enforce this. I guess it's difficult because we use them all over the compiler thread.

> Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:239
> +    if (ident.isCell())
> +        keepAlive(ident.cell());

I'm not sure what I'm missing, but where did this keepAlive come from?

> Source/JavaScriptCore/jit/JITOperations.cpp:2032
> +    if (baseValue.isCell() && CacheableIdentifier::isCacheableIdentifierCell(subscript)) {

If we see a string that's not a cacheable identifier, should we bail on the IC (e.g, make it just go. to the slow path)?

> Source/JavaScriptCore/runtime/CacheableIdentifier.h:89
> +    static constexpr uintptr_t s_uidTag = 1;

nit: Might be worth a comment saying why we tag the UID instead of the cell, since it will be part of conservative GC this way, which is a nice property

> Source/JavaScriptCore/runtime/VM.h:1208
> +    HashMap<const UniquedStringImpl*, RefPtr<WatchpointSet>> m_impurePropertyWatchpointSets;

this looks wrong. You're no longer +1 reffing the string

[Saam Barati]

Comment on attachment 387410 [details]
proposed patch.

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

>> Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:239
>> +        keepAlive(ident.cell());
> 
> I'm not sure what I'm missing, but where did this keepAlive come from?

And if it's legit, why is it needed? Wouldn't writeBarrier make it visible?

[Mark Lam]

Comment on attachment 387410 [details]
proposed patch.

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

>> Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:2
>> + * Copyright (C) 2017-2019 Apple Inc. All rights reserved.
> 
> I think these should be 2020 (you have a few 2019)

Will fix.

>>> Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:239
>>> +        keepAlive(ident.cell());
>> 
>> I'm not sure what I'm missing, but where did this keepAlive come from?
> 
> And if it's legit, why is it needed? Wouldn't writeBarrier make it visible?

keepAlive() comes from HeapCell.h.  This just ensures that the compiler does not optimize away ident until after we barrier the codeBlock because the last use of ident is quite a ways above where we barrier the codeBlock.  That said, we only call this addAccessCase() in tryCacheGetBy() under the protection of GCSafeConcurrentJSLocker.  So, I think it's not strictly needed.  I can remove it.

>> Source/JavaScriptCore/jit/JITOperations.cpp:2032
>> +    if (baseValue.isCell() && CacheableIdentifier::isCacheableIdentifierCell(subscript)) {
> 
> If we see a string that's not a cacheable identifier, should we bail on the IC (e.g, make it just go. to the slow path)?

This is testing for whether the subscript can be made into a CacheableIdentifier i.e symbol or string that is backed by an AtomStringImpl.  It is a refinement on the old isStringOrSymbol() test.  Not sure if we should bail on the IC.  I'll think about it.

>> Source/JavaScriptCore/runtime/CacheableIdentifier.h:89
>> +    static constexpr uintptr_t s_uidTag = 1;
> 
> nit: Might be worth a comment saying why we tag the UID instead of the cell, since it will be part of conservative GC this way, which is a nice property

Will add.

>> Source/JavaScriptCore/runtime/VM.h:1208
>> +    HashMap<const UniquedStringImpl*, RefPtr<WatchpointSet>> m_impurePropertyWatchpointSets;
> 
> this looks wrong. You're no longer +1 reffing the string

Will investigate.

[Saam Barati]

Comment on attachment 387410 [details]
proposed patch.

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

>>> Source/JavaScriptCore/jit/JITOperations.cpp:2032
>>> +    if (baseValue.isCell() && CacheableIdentifier::isCacheableIdentifierCell(subscript)) {
>> 
>> If we see a string that's not a cacheable identifier, should we bail on the IC (e.g, make it just go. to the slow path)?
> 
> This is testing for whether the subscript can be made into a CacheableIdentifier i.e symbol or string that is backed by an AtomStringImpl.  It is a refinement on the old isStringOrSymbol() test.  Not sure if we should bail on the IC.  I'll think about it.

Right. That’s a good point. There’s one slight difference which is this might mutate a string into being an identifier. However, I don’t think it needs to change for this patch.

However, we should have it bail. If you look at the code in the LLInt, it sets profiling data so upper tiers bail if this condition would have failed. So it probably makes sense to be consistent here and have the upper tiers also bail.

I don’t think this patch needs to do this.

[Mark Lam]

(In reply to Mark Lam from comment #22)
> >> Source/JavaScriptCore/jit/JITOperations.cpp:2032
> >> +    if (baseValue.isCell() && CacheableIdentifier::isCacheableIdentifierCell(subscript)) {
> > 
> > If we see a string that's not a cacheable identifier, should we bail on the IC (e.g, make it just go. to the slow path)?
> 
> This is testing for whether the subscript can be made into a
> CacheableIdentifier i.e symbol or string that is backed by an
> AtomStringImpl.  It is a refinement on the old isStringOrSymbol() test.  Not
> sure if we should bail on the IC.  I'll think about it.

The comment was wrong.  I've re-written it.

> >> Source/JavaScriptCore/runtime/VM.h:1208
> >> +    HashMap<const UniquedStringImpl*, RefPtr<WatchpointSet>> m_impurePropertyWatchpointSets;
> > 
> > this looks wrong. You're no longer +1 reffing the string
> 
> Will investigate.

Fixed this by using RefPtr< UniquedStringImpl> instead of const UniquedStringImpl* as the HashMap key.

[Mark Lam]

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

[Saam Barati]

Follow up in:

https://trac.webkit.org/changeset/254496/webkit