[webkit-reviews] review granted: [Bug 199888] Add support for FinalizationRegistries : [Attachment 404620] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 199888: Add support for FinalizationRegistries
https://bugs.webkit.org/show_bug.cgi?id=199888

Attachment 404620: Patch

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




--- Comment #10 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 404620
  --> https://bugs.webkit.org/attachment.cgi?id=404620
Patch

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

r=me

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:936
> +		   531EE7BA22DE6BBB0030DA81 /* FinalizationRegistryPrototype.h
in Headers */ = {isa = PBXBuildFile; fileRef = 531EE7B922DE6BBB0030DA81 /*
FinalizationRegistryPrototype.h */; };
> +		   5333BBDB2110F7D2007618EC /* DFGSpeculativeJIT32_64.cpp in
Sources */ = {isa = PBXBuildFile; fileRef = 86880F1B14328BB900B08D42 /*
DFGSpeculativeJIT32_64.cpp */; };

Some indentation is broken?

> Source/JavaScriptCore/runtime/CommonIdentifiers.h:60
> +    macro(FinalizationRegistry) \

This list is alphabetical sorted, so please move it to "F"'s section.

> Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp:2
> + * Copyright (C) 2017-2019 Apple Inc. All rights reserved.

2020.

> Source/JavaScriptCore/runtime/DeferredWorkTimer.h:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

Ditto.

> Source/JavaScriptCore/runtime/FinalizationRegistryConstructor.cpp:2
> + * Copyright (C) 2019 Apple, Inc. All rights reserved.

Ditto.

> Source/JavaScriptCore/runtime/FinalizationRegistryConstructor.h:2
> + * Copyright (C) 2019 Apple Inc. All rights reserved.

Ditto.

> Source/JavaScriptCore/runtime/FinalizationRegistryConstructor.h:57
> +static_assert(sizeof(FinalizationRegistryConstructor) ==
sizeof(InternalFunction));

Use STATIC_ASSERT_ISO_SUBSPACE_SHARABLE(FinalizationRegistryConstructor,
InternalFunction);

> Source/JavaScriptCore/runtime/FinalizationRegistryPrototype.cpp:2
> + * Copyright (C) 2019 Apple, Inc. All rights reserved.

2020.

> Source/JavaScriptCore/runtime/FinalizationRegistryPrototype.cpp:49
> +    putDirectWithoutTransition(vm, vm.propertyNames->toStringTagSymbol,
jsString(vm, "FinalizationRegistry"), PropertyAttribute::DontEnum |
PropertyAttribute::ReadOnly);

Use JSC_TO_STRING_TAG_WITHOUT_TRANSITION

> Source/JavaScriptCore/runtime/FinalizationRegistryPrototype.h:2
> + * Copyright (C) 2019 Apple, Inc. All rights reserved.

Ditto.

> Source/JavaScriptCore/runtime/IdentifierInlines.h:52
> +	   ASSERT_WITH_MESSAGE(!string.length() || string.impl()->isSymbol() ||
AtomStringImpl::isInAtomStringTable(string.impl()), "The at&omic string comes
from an other thread!");

Let's remove it.

> Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.h:31
> -#include "JSObject.h"
> +#include "IntlObject.h"
>  #include <unicode/ureldatefmt.h>
>  
>  namespace JSC {

Put `enum class RelevantExtensionKey : uint8_t;` forwarding instead.

> Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp:2
> + * Copyright (C) 2019 Apple, Inc. All rights reserved.

Ditto.

> Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp:200
> +    bool result = m_liveRegistrations.remove(token);
> +    result |= m_deadRegistrations.remove(token);

Is it possible that the same token exists on live and dead at the same time? If
it is not possible, can we skip `m_deadRegistrations.remove` when we already
found and removed it in m_liveRegistrations?

> Source/JavaScriptCore/runtime/JSFinalizationRegistry.h:74
> +    static void destroy(JSCell*);

Because of IsoSubspace, we do not need to inherit JSDestructibleObject to make
it destructible. Just define needsDestruction and use
JSInternalFieldObjectImpl.

> Source/JavaScriptCore/runtime/JSInternalFieldObjectImpl.h:80
> +template<unsigned passedNumberOfInternalFields = 1>
> +using JSDestructibleInternalFieldObjectImpl =
JSInternalFieldObjectImpl<passedNumberOfInternalFields, JSDestructibleObject>;

Let's remove these changes since we do not need to use JSDestructibleObject
because of IsoSubspace.

> Source/JavaScriptCore/runtime/JSInternalFieldObjectImplInlines.h:37
> +template<unsigned passedNumberOfInternalFields, typename Base>
> +void JSInternalFieldObjectImpl<passedNumberOfInternalFields,
Base>::visitChildren(JSCell* cell, SlotVisitor& visitor)
>  {
>      auto* thisObject = jsCast<JSInternalFieldObjectImpl*>(cell);
> -    ASSERT_GC_OBJECT_INHERITS(thisObject, info());
> +    ASSERT_GC_OBJECT_INHERITS(thisObject, Base::info());
>      Base::visitChildren(thisObject, visitor);

Ditto.

> Source/JavaScriptCore/runtime/StructureIDTable.cpp:36
> +bool verbose = false;

Let's make it static constexpr bool verbose = false;

> Source/JavaScriptCore/runtime/VM.cpp:1491
> +DYNAMIC_ISO_SUBSPACE_DEFINE_MEMBER_SLOW(finalizationRegistrySpace,
destructibleObjectHeapCellType.get(), JSFinalizationRegistry)

Let's define finalizationRegistryHeapCellType by using IsoHeapCellType::create
and use it.
JSDestructibleObject is now discouraged.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:52
> +    static const unsigned StructureFlags = Base::StructureFlags |
StructureIsImmortal;

I think we should use constexpr.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:87
> +    static const bool needsDestruction = true;

Ditto.