Add support for FinalizationGroups
Created attachment 374345 [details] WIP
Created attachment 404585 [details] Patch
Created attachment 404587 [details] Patch
Created attachment 404589 [details] Patch
Created attachment 404595 [details] Patch
Created attachment 404602 [details] Patch
Created attachment 404609 [details] Patch
Created attachment 404615 [details] Patch
Created attachment 404620 [details] Patch
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.
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?
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?
Created attachment 404733 [details] Patch for landing
Tools/Scripts/svn-apply failed to apply attachment 404733 [details] to trunk. Please resolve the conflicts and upload a new patch.
Created attachment 404735 [details] Patch for landing
Tools/Scripts/svn-apply failed to apply attachment 404735 [details] to trunk. Please resolve the conflicts and upload a new patch.
Created attachment 404736 [details] Patch for landing
Created attachment 404738 [details] Patch for landing
Created attachment 404747 [details] Patch for landing
Created attachment 404748 [details] Patch for landing
Committed r264617: <https://trac.webkit.org/changeset/264617> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404748 [details].
<rdar://problem/65846843>