[webkit-reviews] review granted: [Bug 233878] Split IsoSubspace into a GCClient allocator used by VM and a backend managed by Heap. : [Attachment 452461] proposed patch.

Thu Feb 17 20:38:49 PST 2022

Yusuke Suzuki <ysuzuki at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 233878: Split IsoSubspace into a GCClient allocator used by VM and a
backend managed by Heap.
https://bugs.webkit.org/show_bug.cgi?id=233878

Attachment 452461: proposed patch.

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

--- Comment #8 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 452461
  --> https://bugs.webkit.org/attachment.cgi?id=452461
proposed patch.

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

r=me

> Source/JavaScriptCore/heap/Heap.cpp:3484
> +#undef DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER_SLOW

Can we avoid repeating this list? I think we have the same kind of thing in VM,
is it correct?

> Source/JavaScriptCore/heap/Heap.h:1244
> +class Heap {
> +    WTF_MAKE_NONCOPYABLE(Heap);
> +public:
> +    Heap(JSC::Heap&);
> +    ~Heap();
> +
> +    VM& vm() const;
> +    JSC::Heap& server() { return m_server; }
> +
> +    // FIXME GlobalGC: need a GCClient::Heap::lastChanceToFinalize() and in
there,
> +    // relinquish memory from the IsoSubspace LocalAllocators back to the
server.
> +    // Currently, this is being handled by
BlockDirectory::stopAllocatingForGood().
> +
> +private:
> +    JSC::Heap& m_server;
> +
> +    IsoSubspace arraySpace;
> +    IsoSubspace bigIntSpace;
> +    IsoSubspace calleeSpace;
> +    IsoSubspace clonedArgumentsSpace;
> +    IsoSubspace customGetterSetterSpace;
> +    IsoSubspace dateInstanceSpace;
> +    IsoSubspace domAttributeGetterSetterSpace;
> +    IsoSubspace exceptionSpace;
> +    IsoSubspace executableToCodeBlockEdgeSpace;
> +    IsoSubspace functionSpace;
> +    IsoSubspace getterSetterSpace;
> +    IsoSubspace globalLexicalEnvironmentSpace;
> +    IsoSubspace internalFunctionSpace;
> +    IsoSubspace jsProxySpace;
> +    IsoSubspace nativeExecutableSpace;
> +    IsoSubspace numberObjectSpace;
> +    IsoSubspace plainObjectSpace;
> +    IsoSubspace promiseSpace;
> +    IsoSubspace propertyNameEnumeratorSpace;
> +    IsoSubspace propertyTableSpace;
> +    IsoSubspace regExpSpace;
> +    IsoSubspace regExpObjectSpace;
> +    IsoSubspace ropeStringSpace;
> +    IsoSubspace scopedArgumentsSpace;
> +    IsoSubspace sparseArrayValueMapSpace;
> +    IsoSubspace stringSpace;
> +    IsoSubspace stringObjectSpace;
> +    IsoSubspace structureChainSpace;
> +    IsoSubspace structureRareDataSpace;
> +    IsoSubspace structureSpace;
> +    IsoSubspace brandedStructureSpace;
> +    IsoSubspace symbolTableSpace;
> +
> +#define DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(name) \
> +    template<SubspaceAccess mode> \
> +    IsoSubspace* name() \
> +    { \
> +	   if (m_##name || mode == SubspaceAccess::Concurrently) \
> +	       return m_##name.get(); \
> +	   return name##Slow(); \
> +    } \
> +    JS_EXPORT_PRIVATE IsoSubspace* name##Slow(); \
> +    std::unique_ptr<IsoSubspace> m_##name;
> +
> +#if JSC_OBJC_API_ENABLED
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(apiWrapperObjectSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(objCCallbackFunctionSpace)
> +#endif
> +#ifdef JSC_GLIB_API_ENABLED
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(apiWrapperObjectSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(jscCallbackFunctionSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(callbackAPIWrapperGlobalObjectSpace)
> +#endif
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(apiGlobalObjectSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(apiValueWrapperSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(arrayBufferSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(arrayIteratorSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(asyncGeneratorSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(bigInt64ArraySpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(bigIntObjectSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(bigUint64ArraySpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(booleanObjectSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(boundFunctionSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(callbackConstructorSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(callbackGlobalObjectSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(callbackFunctionSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(callbackObjectSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(customGetterFunctionSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(customSetterFunctionSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(dataViewSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(debuggerScopeSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(errorInstanceSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(float32ArraySpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(float64ArraySpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(functionRareDataSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(generatorSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(globalObjectSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(injectedScriptHostSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(int8ArraySpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(int16ArraySpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(int32ArraySpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(javaScriptCallFrameSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(jsModuleRecordSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(mapBucketSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(mapIteratorSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(mapSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(moduleNamespaceObjectSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(nativeStdFunctionSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(proxyObjectSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(proxyRevokeSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(remoteFunctionSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(scopedArgumentsTableSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(scriptFetchParametersSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(scriptFetcherSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(setBucketSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(setIteratorSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(setSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(shadowRealmSpace);
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(strictEvalActivationSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(stringIteratorSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(sourceCodeSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(symbolSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(symbolObjectSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(templateObjectDescriptorSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(temporalCalendarSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(temporalDurationSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(temporalInstantSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(temporalPlainTimeSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(temporalTimeZoneSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(uint8ArraySpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(uint8ClampedArraySpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(uint16ArraySpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(uint32ArraySpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(unlinkedEvalCodeBlockSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(unlinkedFunctionCodeBlockSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(unlinkedModuleProgramCodeBlockSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(unlinkedProgramCodeBlockSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(finalizationRegistrySpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(weakObjectRefSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(weakSetSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(weakMapSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(withScopeSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(intlCollatorSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(intlDateTimeFormatSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(intlDisplayNamesSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(intlListFormatSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(intlLocaleSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(intlNumberFormatSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(intlPluralRulesSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(intlRelativeTimeFormatSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(intlSegmentIteratorSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(intlSegmenterSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(intlSegmentsSpace)
> +#if ENABLE(WEBASSEMBLY)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(jsToWasmICCalleeSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(webAssemblyExceptionSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(webAssemblyFunctionSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(webAssemblyGlobalSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(webAssemblyInstanceSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(webAssemblyMemorySpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(webAssemblyModuleSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(webAssemblyModuleRecordSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(webAssemblyTableSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(webAssemblyTagSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(webAssemblyWrapperFunctionSpace)
> +#endif
> +
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(evalExecutableSpace)
> +    DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER(moduleProgramExecutableSpace)
> +
> +#undef DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER
> +
> +    IsoSubspace codeBlockSpace;
> +    IsoSubspace functionExecutableSpace;
> +    IsoSubspace programExecutableSpace;
> +    IsoSubspace unlinkedFunctionExecutableSpace;
> +
> +    Vector<IsoSubspacePerVM*> perVMIsoSubspaces;
> +
> +    friend class JSC::VM;
> +    friend class JSC::IsoSubspacePerVM;

Can we avoid repeating this list? I think we have the same kind of thing in VM,
is it correct?