[webkit-reviews] review denied: [Bug 174212] [JSC] Add support for instance class fields : [Attachment 385494] Part 1: Public Instance Fields (rev3)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jan 9 16:41:51 PST 2020
Yusuke Suzuki <ysuzuki at apple.com> has denied Caitlin Potter (:caitp)
<caitp at igalia.com>'s request for review:
Bug 174212: [JSC] Add support for instance class fields
https://bugs.webkit.org/show_bug.cgi?id=174212
Attachment 385494: Part 1: Public Instance Fields (rev3)
https://bugs.webkit.org/attachment.cgi?id=385494&action=review
--- Comment #179 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 385494
--> https://bugs.webkit.org/attachment.cgi?id=385494
Part 1: Public Instance Fields (rev3)
View in context: https://bugs.webkit.org/attachment.cgi?id=385494&action=review
Second round of review. Looks nice overall! But putting r- since we need that
sizeof() of critical classes must be the same.
> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h:420
> + unsigned m_needsClassFieldInitializer : 1;
Can you ensure this does not increase the sizeof(UnlinkedCodeBlock)? This is
strong requirement.
> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:222
> + Optional<Vector<JSTextPosition>> instanceFieldLocations() const
> + {
> + if (m_rareData)
> + return m_rareData->m_instanceFieldLocations;
> + return WTF::nullopt;
> + }
It is copying Vector<>. Can you just return a pointer to
`Vector<JSTextPosition>` instead?
> Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h:259
> + unsigned m_needsClassFieldInitializer : 1;
I think this makes sizeof(UnlinkedFunctionExecutable) larger, and it
immediately consumes super very large size of memory in some website including
Gmail. Please put this flag without increasing
sizeof(UnlinkedFunctionExecutable).
This is strong requirement. I think you can put this after
m_isGeneratedFromCache.
> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3459
> + dst = tempDestination(dst);
Should not accept nullptr here. Caller of this function should ensure it. Just
making this as
OpToPropertyKey::emit(this, dst, src);
return dst;
> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:962
> + func = generator.emitLoadDerivedConstructor();
Let's avoid generator.emitLoadDerivedConstructor if instance-fields do not
exist.
> Source/JavaScriptCore/dfg/DFGCapabilities.cpp:295
> + case op_to_property_key:
File FIXME for DFG handling for this. We need DFG and FTL implementation when
enabling class-fields.
> Source/JavaScriptCore/jit/JITOpcodes.cpp:364
> + int dst = bytecode.m_dst.offset();
> + int src = bytecode.m_src.offset();
Use VirtualRegister.
> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:350
> + int dst = bytecode.m_dst.offset();
> + int src = bytecode.m_src.offset();
Ditto.
> Source/JavaScriptCore/parser/Nodes.h:2239
> + void emitBytecode(BytecodeGenerator&, RegisterID* destination = 0)
override;
Use `= nullptr` for new code.
> Source/JavaScriptCore/parser/Parser.cpp:3049
> + const Identifier* ident = 0;
Use `nullptr` for new code.
> Source/JavaScriptCore/runtime/CodeCache.h:293
> bool isStrictMode = rootNode->features() & StrictModeFeature;
I think using if constexpr is easier to read than using
needsClassFieldInitializer(executable).
NeedsClassFieldInitializer needsClassFieldInitializer =
NeedsClassFieldInitializer::No;
if constexpr (std::is_same_v<ExecutableType, DirectEvalExecutable>)
needsClassFieldInitializer = executable->needsClassFieldInitializer();
> Source/JavaScriptCore/runtime/DirectEvalExecutable.cpp:73
> : EvalExecutable(globalObject, source, inStrictContext,
derivedContextType, isArrowFunctionContext, evalContextType)
Let's propagate needsClassFieldInitializer to EvalExecutable and initialize it
correctly in initializer-list, instead of initializing it inside the
constructor.
More information about the webkit-reviews
mailing list