Changeset 274727 in webkit


Ignore:
Timestamp:
Mar 19, 2021 10:48:47 AM (3 years ago)
Author:
mark.lam@apple.com
Message:

BrandedStructure should keep its members alive.
​https://bugs.webkit.org/show_bug.cgi?id=223495
rdar://75565765

Reviewed by Saam Barati.

JSTests:

  • stress/BrandedStructure-should-keep-its-members-alive.js: Added.

Source/JavaScriptCore:

Normally, each type of JSCell would have its own structure (and therefore, its own
ClassInfo, MethodTable, etc), which would have handled visiting m_parentBrand.
Similarly, it would have its own destructor, which would deref m_brand.

However, the design of BrandedStructure is not like other JSCells. As present,
we have chosen to go with having BrandedStructure look exactly like a regular
Structure, except that its isBrandedStructure flag is set to true.

This design has advantages because we do checks all over the system for whether
a cell is a Structure by simply comparing its structureID to structureStructure's
structureID. By virtue of BrandedStructure having the same structure as Structure,
none of this code need to change.

The downside is that we need to enhance Structure's methods to check if it is
actually working on an instance of BrandedStructure, and do some additional work.

This patch fixes 2 bugs:

  1. m_parentBrand was not visited by visitChildren().

Structure::visitChildrenImpl() now calls BrandedStructure::visitAdditionalChildren()
to handle this.

  1. m_brand needs to be ref'ed.

In Structure::setBrandTransition(), if the BrandedStructure is a dictionary,
then its m_transitionPropertyName will be cleared. m_transitionPropertyName
was the only means by which the UniqueStringImpl pointed to by m_brand was
ref'ed. The fix is to make m_brand a RefPtr.

Hence, it follows that we also need to deref m_brand on destruction.
Structure's destructor now calls BrandedStructure::destruct() to handle this.

  • runtime/BrandedStructure.h:
  • runtime/Structure.cpp:

(JSC::Structure::~Structure):
(JSC::Structure::visitChildrenImpl):

Location:
trunk
Files:
1 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r274703 r274727  
     12021-03-19  Mark Lam  <mark.lam@apple.com>
     2
     3        BrandedStructure should keep its members alive.
     4        https://bugs.webkit.org/show_bug.cgi?id=223495
     5        rdar://75565765
     6
     7        Reviewed by Saam Barati.
     8
     9        * stress/BrandedStructure-should-keep-its-members-alive.js: Added.
     10
    111== Rolled over to ChangeLog-2021-03-18 ==

  • trunk/Source/JavaScriptCore/ChangeLog

    r274724 r274727  
     12021-03-19  Mark Lam  <mark.lam@apple.com>
     2
     3        BrandedStructure should keep its members alive.
     4        https://bugs.webkit.org/show_bug.cgi?id=223495
     5        rdar://75565765
     6
     7        Reviewed by Saam Barati.
     8
     9        Normally, each type of JSCell would have its own structure (and therefore, its own
     10        ClassInfo, MethodTable, etc), which would have handled visiting m_parentBrand.
     11        Similarly, it would have its own destructor, which would deref m_brand.
     12
     13        However, the design of BrandedStructure is not like other JSCells.  As present,
     14        we have chosen to go with having BrandedStructure look exactly like a regular
     15        Structure, except that its isBrandedStructure flag is set to true.
     16
     17        This design has advantages because we do checks all over the system for whether
     18        a cell is a Structure by simply comparing its structureID to structureStructure's
     19        structureID.  By virtue of BrandedStructure having the same structure as Structure,
     20        none of this code need to change.
     21
     22        The downside is that we need to enhance Structure's methods to check if it is
     23        actually working on an instance of BrandedStructure, and do some additional work.
     24
     25        This patch fixes 2 bugs:
     26
     27        1. m_parentBrand was not visited by visitChildren().
     28
     29           Structure::visitChildrenImpl() now calls BrandedStructure::visitAdditionalChildren()
     30           to handle this.
     31
     32        2. m_brand needs to be ref'ed.
     33
     34           In Structure::setBrandTransition(), if the BrandedStructure is a dictionary,
     35           then its m_transitionPropertyName will be cleared.  m_transitionPropertyName
     36           was the only means by which the UniqueStringImpl pointed to by m_brand was
     37           ref'ed.  The fix is to make m_brand a RefPtr.
     38
     39           Hence, it follows that we also need to deref m_brand on destruction.
     40           Structure's destructor now calls BrandedStructure::destruct() to handle this.
     41
     42        * runtime/BrandedStructure.h:
     43        * runtime/Structure.cpp:
     44        (JSC::Structure::~Structure):
     45        (JSC::Structure::visitChildrenImpl):
     46
    1472021-03-19  Sam Weinig  <weinig@apple.com>
    248

  • trunk/Source/JavaScriptCore/runtime/BrandedStructure.h

    r272580 r274727  
    6161    }
    6262
    63 private:
     63    template<typename Visitor>
     64    static void visitAdditionalChildren(JSCell* cell, Visitor& visitor)
     65    {
     66        BrandedStructure* thisObject = jsCast<BrandedStructure*>(cell);
     67        visitor.append(thisObject->m_parentBrand);
     68    }
     69
     70private:
    6471    BrandedStructure(VM&, Structure*, UniquedStringImpl* brand, DeferredStructureTransitionWatchpointFire*);
    6572    BrandedStructure(VM&, BrandedStructure*, DeferredStructureTransitionWatchpointFire*);
    … …  
    6774    static Structure* create(VM&, Structure*, UniquedStringImpl* brand, DeferredStructureTransitionWatchpointFire* = nullptr);
    6875
    69     UniquedStringImpl* m_brand;
     76    void destruct()
     77    {
     78        m_brand = nullptr;
     79    }
     80
     81    RefPtr<UniquedStringImpl> m_brand;
    7082    WriteBarrier<BrandedStructure> m_parentBrand;
    7183

  • trunk/Source/JavaScriptCore/runtime/Structure.cpp

    r273138 r274727  
    343343    if (typeInfo().structureIsImmortal())
    344344        return;
     345
     346    if (isBrandedStructure())
     347        static_cast<BrandedStructure*>(this)->destruct();
    345348    Heap::heap(this)->structureIDTable().deallocateID(this, m_blob.structureID());
    346349}
    … …  
    12961299    else if (thisObject->m_propertyTableUnsafe)
    12971300        thisObject->m_propertyTableUnsafe.clear();
     1301
     1302    if (thisObject->isBrandedStructure())
     1303        BrandedStructure::visitAdditionalChildren(cell, visitor);
    12981304}
    12991305
Note: See TracChangeset for help on using the changeset viewer.