[webkit-reviews] review granted: [Bug 207827] [JSC] Shrink Structure : [Attachment 391488] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 207827: [JSC] Shrink Structure
https://bugs.webkit.org/show_bug.cgi?id=207827

Attachment 391488: Patch

https://bugs.webkit.org/attachment.cgi?id=391488&action=review




--- Comment #23 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 391488
  --> https://bugs.webkit.org/attachment.cgi?id=391488
Patch

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

nice. r=me

Mostly minor nits.

Can you test this on arm64_32 to make sure nothing broke there?

Might also be worth filing a bug for Igalia folks to look into 32-bit on their
architectures

> Source/JavaScriptCore/ChangeLog:8
> +	   This patch shrinks sizeof(Structure) from 112 to 96 (16 bytes) in 64
bit architectures.

it's not 64-bit architectures, its architectures with 64-bit pointers. (Since
arm64_32 does not do this)

> Source/JavaScriptCore/ChangeLog:13
> +	       1. StringHasher is always generating 24 bits hashes so far. By
leveraging this fact,
> +		  we store hash code in Structure in 3 bytes (24 bits) for
property-hash and seen-property-hash.

the code isn't actually doing this. AFAICT, it uses 48 bits for seen
properties, and uses 16 bits to accumulate hash, and then uses the 64-bit
combination of those things as the "seen property hash"

(On a side note, I wonder if we really even need the seen property hash
anymore, since the bloom filter might be sufficient. But I guess if we have the
spare bits, it doesn't hurt to keep it.)

> Source/JavaScriptCore/ChangeLog:20
> +		  We are setting m_cachedPrototypeChain only if Structure is
for JSObject. Clearing happens only if it is not

"are" => "were".
"happens" => "happened"
"it is not" => "it was not"

> Source/JavaScriptCore/ChangeLog:25
> +	   Combining all of the above techniques and gets us 16 bytes.

"and gets us" => "saves us"

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6094
> +		   LValue cachedPrototypeChainOrRareData =
m_out.bitAnd(m_out.constIntPtr(Structure::cachedPrototypeChainOrRareDataMask),
m_out.loadPtr(structure, m_heaps.Structure_cachedPrototypeChainOrRareData));

only do the and if address64?  (and a few other places too in this file)

You could even have a helper that masks out on address64 architectures

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1523
> +#if CPU(ADDRESS64) // Not USE(JSVALUE64). Structure::m_classInfo's
implementation is changed by size of pointer.

not sure this comment is needed given you do this everywhere without this
comment

> Source/JavaScriptCore/runtime/JSObjectInlines.h:219
> +	   [&] (const GCSafeConcurrentJSCellLocker&, PropertyOffset offset,
PropertyOffset newMaxOffset) {

nit: also fix style like you did elsewhere?

> Source/JavaScriptCore/runtime/Structure.h:920
> +    // Structure is one of the most frequently allocated data structure.
Moreover, Structure tends to be alive long!

"alive long" => "alive a long time"

> Source/JavaScriptCore/runtime/Structure.h:929
> +    //  Other pairs works well. We carefully put assertions to setters,
analyze access patterns and pick appropriate pairs in Structure fields.

nice

> Source/JavaScriptCore/runtime/StructureInlines.h:162
> +inline bool Structure::ruleOutUnseenPropertyHash(UniquedStringImpl* uid)
const

this should be called `ruleOutUnseenProperty`. It has nothing to do with
hashes.

> Source/JavaScriptCore/runtime/StructureInlines.h:177
> +#if CPU(ADDRESS64)
> +    return
bitwise_cast<uintptr_t>(m_propertyHashAndSeenProperties.pointer());
> +#else
> +    return m_seenProperties;
> +#endif

minor style nit: I'd make this just return a TinyBloomFilter, and the code
inside ruleOut can just call ruleOut.

> Source/WTF/wtf/CompactPointerTuple.h:57
> +	   return 48 / 8;

should we give 48 a name instead of using it directly everywhere?

Maybe maxNumberOfBitsInPointer?

> Source/WTF/wtf/CompactRefPtrTuple.h:34
> +class CompactRefPtrTuple final {

nice

> LayoutTests/ChangeLog:9
> +	   This test is half-broken since it relies on HashMap's order
implicitly.
> +	   We changed SymbolImpl's hash code, so it makes the result different.

can you file a bug so we can make the test not reliant one this?