Bug 199888 – Add support for FinalizationRegistries

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Add support for FinalizationGroups

[Keith Miller]

Created attachment 374345 [details]
WIP

[Keith Miller]

Created attachment 404585 [details]
Patch

[Keith Miller]

Created attachment 404587 [details]
Patch

[Keith Miller]

Created attachment 404589 [details]
Patch

[Keith Miller]

Created attachment 404595 [details]
Patch

[Keith Miller]

Created attachment 404602 [details]
Patch

[Keith Miller]

Created attachment 404609 [details]
Patch

[Keith Miller]

Created attachment 404615 [details]
Patch

[Keith Miller]

Created attachment 404620 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 404620 [details]
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.

[Yusuke Suzuki]

Comment on attachment 404620 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp:131
> +    if (!vm.deferredWorkTimer->hasPendingWork(this) && (readiedCell || deadCount(NoLockingNecessary))) {

Why not passing locker to deadCount?

[Keith Miller]

Comment on attachment 404620 [details]
Patch

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

>> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:936
>> +                5333BBDB2110F7D2007618EC /* DFGSpeculativeJIT32_64.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 86880F1B14328BB900B08D42 /* DFGSpeculativeJIT32_64.cpp */; };
> 
> Some indentation is broken?

I think it's that my editor doesn't use tabs... Not sure it matters one way or the other though.

>> Source/JavaScriptCore/runtime/CommonIdentifiers.h:60
>> +    macro(FinalizationRegistry) \
> 
> This list is alphabetical sorted, so please move it to "F"'s section.

Done.

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

Whoop, fixed. I should really make a script to do this automatically...

>> 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

Done.

>> 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.

Whoops, done!

>> Source/JavaScriptCore/runtime/IntlRelativeTimeFormat.h:31
>>  namespace JSC {
> 
> Put `enum class RelevantExtensionKey : uint8_t;` forwarding instead.

Good call, done.

>> Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp:131
>> +    if (!vm.deferredWorkTimer->hasPendingWork(this) && (readiedCell || deadCount(NoLockingNecessary))) {
> 
> Why not passing locker to deadCount?

Whoops hold over from when I thought I didn't need the lock.

>> Source/JavaScriptCore/runtime/JSFinalizationRegistry.cpp:200
>> +    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?

Yeah, it's possible, the unregister token can be any JS value and can be used for more than one object at the same time.

>> Source/JavaScriptCore/runtime/JSInternalFieldObjectImpl.h:80
>> +using JSDestructibleInternalFieldObjectImpl = JSInternalFieldObjectImpl<passedNumberOfInternalFields, JSDestructibleObject>;
> 
> Let's remove these changes since we do not need to use JSDestructibleObject because of IsoSubspace.

Done.

>> Source/JavaScriptCore/runtime/JSInternalFieldObjectImplInlines.h:37
>>      Base::visitChildren(thisObject, visitor);
> 
> Ditto.

Done.

>> Source/JavaScriptCore/runtime/StructureIDTable.cpp:36
>> +bool verbose = false;
> 
> Let's make it static constexpr bool verbose = false;

Good catch, fixed.

>> 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.

Gotcha. Didn't know this was the new preferred mechanism, will change.

>> Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.h:52
>> +    static const unsigned StructureFlags = Base::StructureFlags | StructureIsImmortal;
> 
> I think we should use constexpr.

Yeah, I think this was a bad merge. Is there actually a difference though?

[Keith Miller]

Created attachment 404733 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 404733 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Keith Miller]

Created attachment 404735 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 404735 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Keith Miller]

Created attachment 404736 [details]
Patch for landing

[Keith Miller]

Created attachment 404738 [details]
Patch for landing

[Keith Miller]

Created attachment 404747 [details]
Patch for landing

[Keith Miller]

Created attachment 404748 [details]
Patch for landing

[EWS]

Committed r264617: <https://trac.webkit.org/changeset/264617>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404748 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65846843>