[webkit-reviews] review granted: [Bug 194375] [JSC] sizeof(JSString) should be 16 : [Attachment 363077] 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 194375: [JSC] sizeof(JSString) should be 16
https://bugs.webkit.org/show_bug.cgi?id=194375

Attachment 363077: Patch

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




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

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

r=me with comments.

> Source/JavaScriptCore/bytecode/InlineAccess.cpp:169
> +    constexpr bool failIfCantInline = true;

this should go back to being false.

> Source/JavaScriptCore/bytecode/InlineAccess.h:47
> +	   return 26;

Do we need to bump sizes on arm64?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5269
> +    Allocator allocatorValue =
allocatorForNonVirtualConcurrently<JSRopeString>(*m_jit.vm(),
sizeof(JSRopeString), AllocatorForMode::AllocatorIfExists);

Should we assert this isn't null since we're calling AllocatorIfExists but also
assuming it's a constant? E.g, the code below breaks if it's not a constant.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5324
> +	       m_jit.and32(TrustedImm32(string->is8Bit() ?
StringImpl::flagIs8Bit() : 0), scratchGPR);

Why not and16 to be consistent w/ below?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6506
> +    struct FlagsAndLength {
> +	   LValue flags;
> +	   LValue length;
> +    };

style nit: This should go inside compileMakeRope

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6587
> +		       LValue length = lengthCheck;

this is unneeded.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6590
> +			   length

just use lengthCheck here

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6621
> +

Instead of having duplicate code in mergeFlagsAndLength and
initializeFlagsAndLength, I think you could just repeatedly call
initializeFlagAndLengths (and also rename the lambda) and just do:

LValue flags = m_out.bitAnd(flagsAndLength.flags, <flags>);
CheckValue* lengthCheck = m_out.speculateAdd(flagsAndLength.length, <length>));
return FlagsAndLength ...

> Source/JavaScriptCore/runtime/JSString.cpp:245
> +    WTF::storeStoreFence();
> +    new (&uninitializedValueInternal()) String(WTFMove(string));

This is valid today, since this constructor is just a single refptr move.
However, this would be incorrect in the future if sizeof(String) ever changes.
Maybe we should have a static_assert here?

It might be cleaner if we just treated this field like RefPtr<StringImpl>
ourselves.

> Source/JavaScriptCore/runtime/JSString.cpp:246
> +    ASSERT(!JSString::isRope());

Might also be worth a comment saying we don't clear the other bits since we
could be reading the length concurrently.

> Source/JavaScriptCore/runtime/JSString.h:41
> +IGNORE_RETURN_TYPE_WARNINGS_BEGIN

What's this needed for?

> Source/JavaScriptCore/runtime/JSString.h:280
> +    static constexpr uintptr_t flagMask = 0xffff000000000000ULL;
> +    static constexpr uintptr_t stringMask = ~(flagMask | IsRopeInPointer);
> +    static constexpr uintptr_t Is8BitInPointer =
static_cast<uintptr_t>(Is8Bit) << 48;

style nit: capitalization of first letter seems inconsistent here.

> Source/JavaScriptCore/runtime/JSString.h:496
> +	   initializeLength(s1->length() + s2->length() + s3->length());

Seems like this is prior code, but where do we check for overflow?

> Source/JavaScriptCore/runtime/JSString.h:541
> +	   vm.heap.writeBarrier(this, s1);
> +	   vm.heap.writeBarrier(this, s2);

Why? |this| is guaranteed to be in new space here.

> Source/JavaScriptCore/runtime/JSString.h:549
> +	   vm.heap.writeBarrier(this, s1);
> +	   vm.heap.writeBarrier(this, s2);
> +	   vm.heap.writeBarrier(this, s3);

ditto

> Source/JavaScriptCore/runtime/JSString.h:556
> +	   vm.heap.writeBarrier(this, updatedBase);

ditto

> Source/JavaScriptCore/runtime/JSString.h:667
> +	   uintptr_t pointer = bitwise_cast<uintptr_t>(fiber);

let's also assert that !(pointer & ~stringMask)

> Source/JavaScriptCore/runtime/JSString.h:734
> +ALWAYS_INLINE bool JSString::is8Bit() const

It's worth a comment saying this can be run concurrently.

> Source/JavaScriptCore/runtime/JSString.h:750
> +ALWAYS_INLINE unsigned JSString::length() const

ditto

> Source/JavaScriptCore/runtime/JSString.h:754
> +	   return static_cast<const JSRopeString*>(this)->length();

You should say why this is safe to call concurrently. It's because we never
override the length bits when we resolve a rope.

Also nit: use jsCast

> JSTests/stress/to-lower-case-intrinsic-on-empty-rope.js:-24
> -    assert(isRope(rope)); // Keep this test future proofed. If this stops
returning a rope, we should try to find another way to return an empty rope.

Instead of deleting this test, can you just change this to !isRope(rope) and
rename variables appropriately?