Bug 205554 – [JSC] JSFunction's m_executable / m_rareData should be merged

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

[Yusuke Suzuki]

Let's consider introducing one field, m_executableOrRareData to shrink sizeof(JSFunction) from 40 to 32, so that we can remove 16bytes per JSFunction.

[Yusuke Suzuki]

Which field is frequently used?,
1. m_scope is accessed every time CodeBlock is executed (op_get_scope).
2. m_executable access can be skipped if CallLinkInfo well caches the result.

So, we should merge m_rareData & m_executable.

[Yusuke Suzuki]

Gmail allocates 130939 JSFunctions. If we reduce size from 48 to 32, we can save 2~ MB.

[Yusuke Suzuki]

Created attachment 386405 [details]
Patch

[Mark Lam]

Comment on attachment 386405 [details]
Patch

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

Nice.  r=me with some suggestions.

> Source/JavaScriptCore/ChangeLog:10
> +        If we can save sizeof(JSFunction), we can get great gain in memory usage.

/gain/savings/.  "gain" suggests an increase, contrary to your intended meaning.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6567
> +        LValue rareData = m_out.sub(rareDataBits, m_out.constIntPtr(JSFunction::rareDataBit));
>          LValue allocator = m_out.loadPtr(rareData, m_heaps.FunctionRareData_allocator);
>          LValue structure = m_out.loadPtr(rareData, m_heaps.FunctionRareData_structure);

Why not use the same offset trick as you did in the DFG i.e. subtract the rareDataTag (sidenote: see my suggestion for renaming JSFunction::rareDataBit below) from m_heaps.FunctionRareData_allocator and rename it to m_heaps.FunctionRareData_allocator_minus_rareDataTag, and remove the need to adjust the rareDataBits value.  Ditto for FunctionRareData_structure => FunctionRareData_structure_minus_rareDataTag.  From grepping the code, this is the only place that uses these 2 offsets.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6611
> +        LValue rareData = m_out.sub(rareDataBits, m_out.constIntPtr(JSFunction::rareDataBit));
>          LValue structure = m_out.loadPtr(rareData, m_heaps.FunctionRareData_internalFunctionAllocationProfile_structure);

Ditto: FunctionRareData_internalFunctionAllocationProfile_structure => FunctionRareData_internalFunctionAllocationProfile_structure_minus_rareDataTag.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6666
> +        LValue rareData = m_out.sub(rareDataBits, m_out.constIntPtr(JSFunction::rareDataBit));
>          LValue structure = m_out.loadPtr(rareData, m_heaps.FunctionRareData_internalFunctionAllocationProfile_structure);

Ditto: FunctionRareData_internalFunctionAllocationProfile_structure => FunctionRareData_internalFunctionAllocationProfile_structure_minus_rareDataTag.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1150
> +.executable:

nit: can you rename this label to .isExecutable?  Ditto for the cases below.

> Source/JavaScriptCore/runtime/JSFunction.cpp:133
> -void JSFunction::finishCreation(VM& vm, NativeExecutable* executable, int length, const String& name)
> +void JSFunction::finishCreation(VM& vm, NativeExecutable*, int length, const String& name)

Why not remove the NativeExecutable* argument altogether?

> Source/JavaScriptCore/runtime/JSFunction.h:64
> +    static constexpr uintptr_t rareDataBit = 0x1;

nit: can you rename this as rareDataTag?  The term "tag" when used on the executableOrRareData pointer tells us right away that we're tagging the pointer (which is an immediately understood concept), whereas a "bit" applied to a pointer doesn't have the same context.  Hence, I think "tag" is a better name to use here.

> Source/JavaScriptCore/runtime/JSFunction.h:129
>      FunctionRareData* rareData(VM& vm)

nit: this is not due to your changes, but this function should really be renamed to ensureRareData() by WebKit convention.  Can you do this rename in a follow up patch?  Doing this rename will also help further disambiguate it from the other rareData() function below that does not instantiate a FunctionRareData instance.

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:104
> +    uintptr_t executableOrRareData = m_executableOrRareData;
> +    if (executableOrRareData & rareDataBit)
> +        return bitwise_cast<FunctionRareData*>(executableOrRareData & ~rareDataBit)->hasReifiedLength();

nit: It would be nice to use JSFunction::rareData() (here instead of doing the pointer tag check and untagging ourselves).  The only extra cost is the loadLoadFence().  Since hasReifiedLength() is not used in any perf critical paths, it might be more cleaner to use the rareData() method instead.

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:113
> +    uintptr_t executableOrRareData = m_executableOrRareData;
> +    if (executableOrRareData & rareDataBit)
> +        return bitwise_cast<FunctionRareData*>(executableOrRareData & ~rareDataBit)->hasReifiedName();
> +    return false;

Ditto: use the rareData() method instead?

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:121
> +    uintptr_t executableOrRareData = m_executableOrRareData;
> +    if (!(executableOrRareData & rareDataBit))
>          return true;
> -    if (m_rareData->hasModifiedName())
> +    FunctionRareData* rareData = bitwise_cast<FunctionRareData*>(executableOrRareData & ~rareDataBit);

Ditto: Would be nice to use the rareData() method here too.

> Source/JavaScriptCore/runtime/JSFunctionInlines.h:153
> -    if (UNLIKELY(!m_rareData))
> +    uintptr_t executableOrRareData = m_executableOrRareData;
> +    if (!(executableOrRareData & rareDataBit))

Can also rareData() method here.  I think the code will read cleaner that way.

[Yusuke Suzuki]

Comment on attachment 386405 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:10
>> +        If we can save sizeof(JSFunction), we can get great gain in memory usage.
> 
> /gain/savings/.  "gain" suggests an increase, contrary to your intended meaning.

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6567
>>          LValue structure = m_out.loadPtr(rareData, m_heaps.FunctionRareData_structure);
> 
> Why not use the same offset trick as you did in the DFG i.e. subtract the rareDataTag (sidenote: see my suggestion for renaming JSFunction::rareDataBit below) from m_heaps.FunctionRareData_allocator and rename it to m_heaps.FunctionRareData_allocator_minus_rareDataTag, and remove the need to adjust the rareDataBits value.  Ditto for FunctionRareData_structure => FunctionRareData_structure_minus_rareDataTag.  From grepping the code, this is the only place that uses these 2 offsets.

It is a bit hard to represent this in our TBAA heap access offsets. And I think B3 can optimize this (This is why we are using sub instead of bitAnd, which makes B3 easily optimize this).

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6611
>>          LValue structure = m_out.loadPtr(rareData, m_heaps.FunctionRareData_internalFunctionAllocationProfile_structure);
> 
> Ditto: FunctionRareData_internalFunctionAllocationProfile_structure => FunctionRareData_internalFunctionAllocationProfile_structure_minus_rareDataTag.

Ditto.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6666
>>          LValue structure = m_out.loadPtr(rareData, m_heaps.FunctionRareData_internalFunctionAllocationProfile_structure);
> 
> Ditto: FunctionRareData_internalFunctionAllocationProfile_structure => FunctionRareData_internalFunctionAllocationProfile_structure_minus_rareDataTag.

Ditto.

>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:1150
>> +.executable:
> 
> nit: can you rename this label to .isExecutable?  Ditto for the cases below.

Fixed.

>> Source/JavaScriptCore/runtime/JSFunction.cpp:133
>> +void JSFunction::finishCreation(VM& vm, NativeExecutable*, int length, const String& name)
> 
> Why not remove the NativeExecutable* argument altogether?

This `finishCreation` is only used for JSFunction holding NativeExecutable*, and this type is used as a tag to distinguish this between multiple finishCreation implementations.

>> Source/JavaScriptCore/runtime/JSFunction.h:64
>> +    static constexpr uintptr_t rareDataBit = 0x1;
> 
> nit: can you rename this as rareDataTag?  The term "tag" when used on the executableOrRareData pointer tells us right away that we're tagging the pointer (which is an immediately understood concept), whereas a "bit" applied to a pointer doesn't have the same context.  Hence, I think "tag" is a better name to use here.

Sounds nice. Changed.

>> Source/JavaScriptCore/runtime/JSFunction.h:129
>>      FunctionRareData* rareData(VM& vm)
> 
> nit: this is not due to your changes, but this function should really be renamed to ensureRareData() by WebKit convention.  Can you do this rename in a follow up patch?  Doing this rename will also help further disambiguate it from the other rareData() function below that does not instantiate a FunctionRareData instance.

Sounds nice. Fixed.

>> Source/JavaScriptCore/runtime/JSFunctionInlines.h:104
>> +        return bitwise_cast<FunctionRareData*>(executableOrRareData & ~rareDataBit)->hasReifiedLength();
> 
> nit: It would be nice to use JSFunction::rareData() (here instead of doing the pointer tag check and untagging ourselves).  The only extra cost is the loadLoadFence().  Since hasReifiedLength() is not used in any perf critical paths, it might be more cleaner to use the rareData() method instead.

Currently, I would like to keep this since `JSFunction::rareData()` includes loadLoadFence() while it is not necessary.
I would like to put FIXME here, and later, I would remove this by removing `WTF::loadLoadFence()` in `rareData`.

>> Source/JavaScriptCore/runtime/JSFunctionInlines.h:113
>> +    return false;
> 
> Ditto: use the rareData() method instead?

Ditto.

>> Source/JavaScriptCore/runtime/JSFunctionInlines.h:121
>> +    FunctionRareData* rareData = bitwise_cast<FunctionRareData*>(executableOrRareData & ~rareDataBit);
> 
> Ditto: Would be nice to use the rareData() method here too.

Ditto.

>> Source/JavaScriptCore/runtime/JSFunctionInlines.h:153
>> +    if (!(executableOrRareData & rareDataBit))
> 
> Can also rareData() method here.  I think the code will read cleaner that way.

Ditto.

[Yusuke Suzuki]

Committed r253932: <https://trac.webkit.org/changeset/253932>

[Radar WebKit Bug Importer]

<rdar://problem/58228681>

[Yusuke Suzuki]

Committed r253933: <https://trac.webkit.org/changeset/253933>

[Saam Barati]

Comment on attachment 386405 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12422
> +    m_jit.loadPtr(CCallHelpers::Address(resultGPR, FunctionRareData::offsetOfExecutable() - JSFunction::rareDataBit), resultGPR);

so do we just use 7 bytes to store the pointer? And the lower byte is either one or zero? 

I guess I'm a little confused as to how this works, since what if the byte is non zero?

[Saam Barati]

Comment on attachment 386405 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:12422
>> +    m_jit.loadPtr(CCallHelpers::Address(resultGPR, FunctionRareData::offsetOfExecutable() - JSFunction::rareDataBit), resultGPR);
> 
> so do we just use 7 bytes to store the pointer? And the lower byte is either one or zero? 
> 
> I guess I'm a little confused as to how this works, since what if the byte is non zero?

I got confused, this is loading off of FunctionRareData. I see why this works now, since if it's rare data, the pointer is offset by 1. Nice, this is cool.

[Saam Barati]

Comment on attachment 386405 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:18
> +        This patch reduces sizeof(JSFunction) from 48 to 32.

👍🏼

[Yusuke Suzuki]

This improves RAMification by ~0.5%, and it also improves new Membuster by 1%.