Bug 195638 – [JSC] CodeBlock::visitChildren is reporting extra memory even when its JITCode is singleton

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

[Caio Lima]

After https://bugs.webkit.org/show_bug.cgi?id=194659, we have singleton DirectJITCode per entrypoint type on no JIT configuration. However, we still use setJITCode and keep reporting extra memory for those JITCodes, even if they being shared.
Extra memory is used by `Heap::updateAllocationLimits` and having higher value can potentially put more pressure on GC.

[Caio Lima]

Created attachment 364435 [details]
RFC Patch

This change was used to create an alternative method to notify CodeBlock that it is referring to a shared m_jitCode. The flag `m_isSharedJITCode` is used to store such modification.

We ran Speedometer on minibrowser with JIT disabled to compare this approach. The results there are neutral.

Run 1:
ToT: 55.8 +- 1.0
Changes: 55.7 +- 1.0

Run 2:
ToT: 53.1 +- 0.69
Changes: 53.6 +- 0.49

Run 3:
ToT: 50.04 +- 0.65
Changes: 49.8 +- 0.58

I tried run some numbers on JetStream2, but with no JIT config, takes very long to run tests. It is probably very hard to see performance difference there, but I'll run other JS benchmarks to see difference on numbers.

[Caio Lima]

Created attachment 364534 [details]
WIP - Patch

[Dominik Inführ]

Looks good to me.

[Mark Lam]

Comment on attachment 364534 [details]
WIP - Patch

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

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:951
> +    if (thisObject->m_jitCode && !thisObject->m_jitCode->isShared())
>          extraMemoryAllocated += thisObject->m_jitCode->size();

nit: we're computing thisObject->m_jitCode 3 times here.  I suggest prefetching it once.

> Source/JavaScriptCore/bytecode/CodeBlock.h:395
> +            ASSERT(heap()->isDeferred());

Do we really want to make this assert conditional?  Even though we only reportExtraMemoryAllocated() when !isShared, do we ever want to allow the caller to call setJITCode() without deferring GCs?  Unless there's a strong reason why we need this,I think it's better to have that assertion unconditionally so that the contract for calling setJITCode() is unconditional.

> Source/JavaScriptCore/jit/JITCode.h:208
> +    virtual bool isShared() const { return false; }

I'm tend to be wary of adding virtual functions unless there's a good reason for it.  In this case, it probably doesn't matter much either way.  However, I think we can solve this a different way by adding a m_isShared bool to class JITCode itself.  There's definitely space for it in the object already.  That way, we can avoid creating 2 new classes, and some virtual function calls.  Actually, instead of a bool, the preferred idiom would be to use a:

    class JITCode {
        ....
        enum class ShareAttribute : uint8_t {
            NotShared,
            Shared
        };

... and have the JITCode constructor take a default arg of ShareAttribute::NotShared.  The shared cases can pass ShareAttribute:Shared explicitly.

> Source/JavaScriptCore/jit/JITCode.h:252
> +    SharedDirectJITCode(CodeRef<JSEntryPtrTag>, CodePtr<JSEntryPtrTag> withArityCheck, JITType);

I recommend just implementing the constructor as an inline function here instead of in the .cpp.  All it does is forward construction to the base class.  Hence, there's no value in not inlining it.

> Source/JavaScriptCore/jit/JITCode.h:253
> +    virtual ~SharedDirectJITCode();

This is not needed since it adds no behavior change.

> Source/JavaScriptCore/jit/JITCode.h:269
> +    SharedNativeJITCode(CodeRef<JSEntryPtrTag>, JITType);

Ditto: inline the constructor.

> Source/JavaScriptCore/jit/JITCode.h:270
> +    virtual ~SharedNativeJITCode();

Ditto: not needed.

[Caio Lima]

Created attachment 364574 [details]
Patch

[Caio Lima]

Comment on attachment 364534 [details]
WIP - Patch

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

Thx a lot for the review. I uploaded new Patch with your comments.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:951
>>          extraMemoryAllocated += thisObject->m_jitCode->size();
> 
> nit: we're computing thisObject->m_jitCode 3 times here.  I suggest prefetching it once.

Done

>> Source/JavaScriptCore/bytecode/CodeBlock.h:395
>> +            ASSERT(heap()->isDeferred());
> 
> Do we really want to make this assert conditional?  Even though we only reportExtraMemoryAllocated() when !isShared, do we ever want to allow the caller to call setJITCode() without deferring GCs?  Unless there's a strong reason why we need this,I think it's better to have that assertion unconditionally so that the contract for calling setJITCode() is unconditional.

No reason. I moved ASSERT outside the loop then.

>> Source/JavaScriptCore/jit/JITCode.h:208
>> +    virtual bool isShared() const { return false; }
> 
> I'm tend to be wary of adding virtual functions unless there's a good reason for it.  In this case, it probably doesn't matter much either way.  However, I think we can solve this a different way by adding a m_isShared bool to class JITCode itself.  There's definitely space for it in the object already.  That way, we can avoid creating 2 new classes, and some virtual function calls.  Actually, instead of a bool, the preferred idiom would be to use a:
> 
>     class JITCode {
>         ....
>         enum class ShareAttribute : uint8_t {
>             NotShared,
>             Shared
>         };
> 
> ... and have the JITCode constructor take a default arg of ShareAttribute::NotShared.  The shared cases can pass ShareAttribute:Shared explicitly.

Ok. The reason to use virtual methods was to avoid adding a new field to JITCode, but since it is not a problem here, I moved to the approach you suggested.

>> Source/JavaScriptCore/jit/JITCode.h:252
>> +    SharedDirectJITCode(CodeRef<JSEntryPtrTag>, CodePtr<JSEntryPtrTag> withArityCheck, JITType);
> 
> I recommend just implementing the constructor as an inline function here instead of in the .cpp.  All it does is forward construction to the base class.  Hence, there's no value in not inlining it.

Not necessary, since I removed this class.

>> Source/JavaScriptCore/jit/JITCode.h:253
>> +    virtual ~SharedDirectJITCode();
> 
> This is not needed since it adds no behavior change.

Ditto.

>> Source/JavaScriptCore/jit/JITCode.h:269
>> +    SharedNativeJITCode(CodeRef<JSEntryPtrTag>, JITType);
> 
> Ditto: inline the constructor.

Ditto.

>> Source/JavaScriptCore/jit/JITCode.h:270
>> +    virtual ~SharedNativeJITCode();
> 
> Ditto: not needed.

Ditto.

[Caio Lima]

Created attachment 364576 [details]
Patch

[Mark Lam]

Comment on attachment 364574 [details]
Patch

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

LGTM.  Please remove the test code before landing.

> Source/JavaScriptCore/ChangeLog:11
> +        This patch is introducing new field on JITCode to represent
> +        its shared versions. With this new field, we introduced a new
> +        method `isShared()` to indicate that a JITCode is shared among
> +        CodeBlocks. This is being used by `CodeBlock::setJITCode` and

I suggest rephrasing the above as:
This patch introduces a m_isShared flag to track whether the JITCode is shared between  many CodeBlocks.  This flag is used in `CodeBlock::setJITCode ...

> Source/JavaScriptCore/ChangeLog:12
> +        `CodeBlock::visitChildren` to avoid report duplicated extra memory for

/report duplicated/reporting duplicate/

> Source/JavaScriptCore/ChangeLog:14
> +        With those changes, we now stop accounting singleton LLIntEntrypoints

... we now stop counting singleton ...

> Source/JavaScriptCore/ChangeLog:16
> +        change can pottentially avoid unecessary GC pressure, because

/pottentially/potentially/

> Source/JavaScriptCore/ChangeLog:20
> +        (see results below), it is important keep extra memory usage consistent,

important to ...

I would also say "correct" instead of "consistent".  I also suggest putting a period after consistent/correct, and starting "Otherwise ..." as a new sentence.

> Source/JavaScriptCore/jit/JITCode.h:216
> +    bool isShared() const
> +    { 
> +        return m_shareAttribute == ShareAttribute::Shared;
> +    }

nit: I prefer to put this all on a single line since there's only one statement in it.  For example, see intrinsic() above.

> Source/JavaScriptCore/runtime/Options.cpp:226
>  static bool jitEnabledByDefault()
>  {
> -    return is32Bit() || isAddress64Bit();
> +    return false;
>  }

I think this is test code you forgot to remove.

[Mark Lam]

Comment on attachment 364576 [details]
Patch

LGTM but please see my suggestions from https://bugs.webkit.org/show_bug.cgi?id=195638#c8.

[Caio Lima]

Comment on attachment 364574 [details]
Patch

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

Thx a lot for the review and suggestions.

>> Source/JavaScriptCore/ChangeLog:11
>> +        CodeBlocks. This is being used by `CodeBlock::setJITCode` and
> 
> I suggest rephrasing the above as:
> This patch introduces a m_isShared flag to track whether the JITCode is shared between  many CodeBlocks.  This flag is used in `CodeBlock::setJITCode ...

Done.

>> Source/JavaScriptCore/ChangeLog:16
>> +        change can pottentially avoid unecessary GC pressure, because
> 
> /pottentially/potentially/

Oops.

>> Source/JavaScriptCore/ChangeLog:20
>> +        (see results below), it is important keep extra memory usage consistent,
> 
> important to ...
> 
> I would also say "correct" instead of "consistent".  I also suggest putting a period after consistent/correct, and starting "Otherwise ..." as a new sentence.

Changed.

>> Source/JavaScriptCore/jit/JITCode.h:216
>> +    }
> 
> nit: I prefer to put this all on a single line since there's only one statement in it.  For example, see intrinsic() above.

Done.

>> Source/JavaScriptCore/runtime/Options.cpp:226
>>  }
> 
> I think this is test code you forgot to remove.

Oops. I uploaded a version without this. I'm sorry for the noise.

[Caio Lima]

Created attachment 364594 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 364594 [details]
Patch

Clearing flags on attachment: 364594

Committed r242928: <https://trac.webkit.org/changeset/242928>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48872196>