[webkit-reviews] review granted: [Bug 110854] Unused Structure property tables waste 14MB on Membuster. : [Attachment 190221] Proposed patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Andreas Kling <akling at apple.com>'s
request for review:
Bug 110854: Unused Structure property tables waste 14MB on Membuster.
https://bugs.webkit.org/show_bug.cgi?id=110854

Attachment 190221: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=190221&action=review

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=190221&action=review


Basically, r=me, but with the slight changes.

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:280
> +inline PropertyTable* PropertyTable::create(JSGlobalData& globalData,
unsigned initialCapacity)
> +{
> +    PropertyTable* table = new (NotNull,
allocateCell<PropertyTable>(globalData.heap)) PropertyTable(globalData,
initialCapacity);
> +    table->finishCreation(globalData);
> +    return table;
> +}
> +
> +inline PropertyTable* PropertyTable::clone(JSGlobalData& globalData, JSCell*
owner, const PropertyTable& other)
> +{
> +    PropertyTable* table = new (NotNull,
allocateCell<PropertyTable>(globalData.heap)) PropertyTable(globalData, owner,
other);
> +    table->finishCreation(globalData);
> +    return table;
> +}
> +
> +inline PropertyTable* PropertyTable::clone(JSGlobalData& globalData, JSCell*
owner, unsigned initialCapacity, const PropertyTable& other)
> +{
> +    PropertyTable* table = new (NotNull,
allocateCell<PropertyTable>(globalData.heap)) PropertyTable(globalData, owner,
initialCapacity, other);
> +    table->finishCreation(globalData);
> +    return table;
> +}
> +
> +inline PropertyTable::PropertyTable(JSGlobalData& globalData, unsigned
initialCapacity)
> +    : JSCell(globalData, globalData.propertyTableStructure.get())
> +    , m_indexSize(sizeForCapacity(initialCapacity))

While you're creating PropertyMapHashTable.cpp, you could out-of-line these
methods.  They do no good being inline.

> Source/JavaScriptCore/runtime/Structure.cpp:78
> +const ClassInfo PropertyTable::s_info = { "PropertyTable", 0, 0, 0,
CREATE_METHOD_TABLE(PropertyTable) };
> +
> +void PropertyTable::visitChildren(JSCell* cell, SlotVisitor& visitor)
> +{
> +    PropertyTable* thisObject = jsCast<PropertyTable*>(cell);
> +    ASSERT_GC_OBJECT_INHERITS(thisObject, &s_info);
> +    ASSERT(thisObject->structure()->typeInfo().overridesVisitChildren());
> +
> +    JSCell::visitChildren(thisObject, visitor);
> +
> +    PropertyTable::iterator end = thisObject->end();
> +    for (PropertyTable::iterator ptr = thisObject->begin(); ptr != end;
++ptr)
> +	   visitor.append(&ptr->specificValue);
> +}
> +
>  #if DUMP_STRUCTURE_ID_STATISTICS
>  static HashSet<Structure*>& liveStructureSet = *(new HashSet<Structure*>);

These should be in PropertyMapHashTable.cpp.  Or just PropertyTable.cpp.  Your
pick of name.  But I wouldn't pollute Structure.cpp with these things.

(I know, adding .cpp files to WebKit is soooper hard, but at least here you
don't have to do gyp.  So one less build system.)

> Source/JavaScriptCore/runtime/StructureInlines.h:200
> +inline bool Structure::putWillGrowOutOfLineStorage()
> +{
> +    checkOffsetConsistency();
> +
> +    ASSERT(outOfLineCapacity() >= outOfLineSize());
> +
> +    if (!m_propertyTable) {
> +	   unsigned currentSize =
numberOfOutOfLineSlotsForLastOffset(m_offset);
> +	   ASSERT(outOfLineCapacity() >= currentSize);
> +	   return currentSize == outOfLineCapacity();
> +    }
> +
> +    ASSERT(totalStorageCapacity() >=
m_propertyTable->propertyStorageSize());
> +    if (m_propertyTable->hasDeletedOffset())
> +	   return false;
> +
> +    ASSERT(totalStorageCapacity() >= m_propertyTable->size());
> +    return m_propertyTable->size() == totalStorageCapacity();

Might as well drop this method into Structure.cpp instead of having it be
inline.  It meets the basic criteria for not being inline: it's not called
often enough and it has a fair bit of code (multiple loads and branches).