Bug 154838 – Add new MethodTable method to get an estimated size for a cell

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

[Joseph Pecoraro]

* SUMMARY
Add new MethodTable method to get an estimated size for a cell

The new class method estimatedSize(JSCell*) estimates the size for a single cell.

    Base implementation (JSCell)
      - returns the MarkedBlock bucket size for this cell.
      - This gets us basic member/inline storage. Basically a better sizeof.

    Subclasses with "Extra Memory Cost":
      - Any class that reports extra memory (reportExtraMemoryVisited) should include that in the estimated size.
      - E.g. CodeBlock, JSGenericTypedArrayView, WeakMapData, etc.

    Subclasses with "Copied Space" storage:
      - Any class with data in copied space (has copyBackingStore) should include that in the estimated size.
      - E.g. JSObject, JSGenericTypedArrayView, JSMap, JSSet, DirectArguments, etc.

* NOTES
To be used by Inspector for JavaScript Heap Instrumentation.

[Joseph Pecoraro]

Created attachment 272525 [details]
[PATCH] Proposed Fix

[Mark Lam]

Comment on attachment 272525 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2468
> +    size_t extraMemoryAllocated = thisObject->m_instructions.size() * sizeof(Instruction);

Maybe add a fixme here (with a bug to track) to add JITCode size (for baseline and DFG/FTL codeBlocks)?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:416
> +    if (thisObject->m_mode == OversizeTypedArray)
> +        return Base::estimatedSize(thisObject) + thisObject->byteSize();
> +    if (thisObject->m_mode == FastTypedArray && thisObject->m_vector)
> +        return Base::estimatedSize(thisObject) + thisObject->byteSize();

Why not || the 2 conditions?

[Joseph Pecoraro]

(In reply to comment #2)
> Comment on attachment 272525 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272525&action=review
> 
> > Source/JavaScriptCore/bytecode/CodeBlock.cpp:2468
> > +    size_t extraMemoryAllocated = thisObject->m_instructions.size() * sizeof(Instruction);

Good catch, I should definitely have been including the m_jitCode->size() here if we have m_jitCode.


> Maybe add a fixme here (with a bug to track) to add JITCode size (for
> baseline and DFG/FTL codeBlocks)?
> 
> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:416
> > +    if (thisObject->m_mode == OversizeTypedArray)
> > +        return Base::estimatedSize(thisObject) + thisObject->byteSize();
> > +    if (thisObject->m_mode == FastTypedArray && thisObject->m_vector)
> > +        return Base::estimatedSize(thisObject) + thisObject->byteSize();
> 
> Why not || the 2 conditions?

I could go either way on this. I converted the switch in JSGenericTypedArrayView<Adaptor>::visitChildren to a couple if statements. They happened to have the same calculation, but most of the code in this file treats the modes separate (via the switch), so I followed suit here. I like treating these as separate in case storage ever differs between the two different modes.

[Joseph Pecoraro]

Created attachment 272532 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 272532 [details]
[PATCH] For Landing

Clearing flags on attachment: 272532

Committed r197379: <http://trac.webkit.org/changeset/197379>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.