[webkit-reviews] review granted: [Bug 205554] [JSC] JSFunction's m_executable / m_rareData should be merged : [Attachment 386405] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 205554: [JSC] JSFunction's m_executable / m_rareData should be merged
https://bugs.webkit.org/show_bug.cgi?id=205554

Attachment 386405: Patch

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




--- Comment #4 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 386405
  --> https://bugs.webkit.org/attachment.cgi?id=386405
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.