Skip to content

Commit

Permalink
Bug 1629796: Replace finalization iterator with multiple callback cal…
Browse files Browse the repository at this point in the history 
…ls. r=jonco

Implements the spec changes from: tc39/proposal-weakrefs#187

The spec change removes the `FinalizationRegistryCleanupIterator` in favour of
calling the clean-up callback for each finalised value. It also allows to call
`cleanupSome()` within the callback function.

`FinalizationRegistryObject::cleanupQueuedRecords()` has been changed to iterate
from back to front, because this allows us to call `GCVector::popCopy()`, which
makes it more efficient to remove entries from the `records` vector.

Differential Revision: https://phabricator.services.mozilla.com/D70821
  • Loading branch information
@anba
anba committed May 13, 2020
1 parent aeb85f1 commit 3a4567b
Show file tree
Hide file tree
Showing 13 changed files with 121 additions and 336 deletions.
2 changes: 1 addition & 1 deletion js/public/Class.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -700,7 +700,7 @@ static const uint32_t JSCLASS_FOREGROUND_FINALIZE =
// application.
static const uint32_t JSCLASS_GLOBAL_APPLICATION_SLOTS = 5;
static const uint32_t JSCLASS_GLOBAL_SLOT_COUNT =
JSCLASS_GLOBAL_APPLICATION_SLOTS + JSProto_LIMIT * 2 + 26;
JSCLASS_GLOBAL_APPLICATION_SLOTS + JSProto_LIMIT * 2 + 25;

static constexpr uint32_t JSCLASS_GLOBAL_FLAGS_WITH_SLOTS(uint32_t n) {
return JSCLASS_IS_GLOBAL |
Expand Down
255 changes: 22 additions & 233 deletions js/src/builtin/FinalizationRegistryObject.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,6 @@ bool FinalizationRegistryObject::construct(JSContext* cx, unsigned argc,
recordsToBeCleanedUp.release(),
MemoryUse::FinalizationRegistryRecordVector);
registry->initReservedSlot(IsQueuedForCleanupSlot, BooleanValue(false));
registry->initReservedSlot(IsCleanupJobActiveSlot, BooleanValue(false));

if (!cx->runtime()->gc.addFinalizationRegistry(cx, registry)) {
return false;
Expand Down Expand Up @@ -439,10 +438,6 @@ bool FinalizationRegistryObject::isQueuedForCleanup() const {
return getReservedSlot(IsQueuedForCleanupSlot).toBoolean();
}

bool FinalizationRegistryObject::isCleanupJobActive() const {
return getReservedSlot(IsCleanupJobActiveSlot).toBoolean();
}

void FinalizationRegistryObject::queueRecordToBeCleanedUp(
FinalizationRecordObject* record) {
AutoEnterOOMUnsafeRegion oomUnsafe;
Expand All @@ -456,11 +451,6 @@ void FinalizationRegistryObject::setQueuedForCleanup(bool value) {
setReservedSlot(IsQueuedForCleanupSlot, BooleanValue(value));
}

void FinalizationRegistryObject::setCleanupJobActive(bool value) {
MOZ_ASSERT(value != isCleanupJobActive());
setReservedSlot(IsCleanupJobActiveSlot, BooleanValue(value));
}

// FinalizationRegistry.prototype.register(target, heldValue [, unregisterToken
// ])
// https://tc39.es/proposal-weakrefs/#sec-finalization-registry.prototype.register
Expand Down Expand Up @@ -705,11 +695,7 @@ bool FinalizationRegistryObject::cleanupSome(JSContext* cx, unsigned argc,
CallArgs args = CallArgsFromVp(argc, vp);

// 1. Let finalizationRegistry be the this value.
// 2. If Type(finalizationRegistry) is not Object, throw a TypeError
// exception.
// 3. If finalizationRegistry does not have [[Cells]] and
// [[IsFinalizationRegistryCleanupJobActive]] internal slots, throw a
// TypeError exception.
// 2. Perform ? RequireInternalSlot(finalizationRegistry, [[Cells]]).
if (!args.thisv().isObject() ||
!args.thisv().toObject().is<FinalizationRegistryObject>()) {
JS_ReportErrorNumberASCII(
Expand All @@ -718,17 +704,10 @@ bool FinalizationRegistryObject::cleanupSome(JSContext* cx, unsigned argc,
return false;
}

// 4. If finalizationRegistry.[[IsFinalizationRegistryCleanupJobActive]] is
// true, throw a TypeError exception.
RootedFinalizationRegistryObject registry(
cx, &args.thisv().toObject().as<FinalizationRegistryObject>());
if (registry->isCleanupJobActive()) {
JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
JSMSG_BAD_CLEANUP_STATE);
return false;
}

// 5. If callback is not undefined and IsCallable(callback) is false, throw a
// 3. If callback is not undefined and IsCallable(callback) is false, throw a
// TypeError exception.
RootedObject cleanupCallback(cx);
if (!args.get(0).isUndefined()) {
Expand All @@ -746,24 +725,6 @@ bool FinalizationRegistryObject::cleanupSome(JSContext* cx, unsigned argc,
return true;
}

/* static */
bool FinalizationRegistryObject::hasRegisteredRecordsToBeCleanedUp(
HandleFinalizationRegistryObject registry) {
FinalizationRecordVector* records = registry->recordsToBeCleanedUp();
size_t initialLength = records->length();
if (initialLength == 0) {
return false;
}

for (FinalizationRecordObject* record : *records) {
if (record->isActive()) {
return true;
}
}

return false;
}

// CleanupFinalizationRegistry ( finalizationRegistry [ , callback ] )
// https://tc39.es/proposal-weakrefs/#sec-cleanup-finalization-registry
/* static */
Expand All @@ -772,20 +733,7 @@ bool FinalizationRegistryObject::cleanupQueuedRecords(
HandleObject callbackArg) {
MOZ_ASSERT(cx->compartment() == registry->compartment());

// 2. If CheckForEmptyCells(finalizationRegistry) is false, return.
if (!hasRegisteredRecordsToBeCleanedUp(registry)) {
return true;
}

// 3. Let iterator be
// !CreateFinalizationRegistryCleanupIterator(finalizationRegistry).
Rooted<FinalizationIteratorObject*> iterator(
cx, FinalizationIteratorObject::create(cx, registry));
if (!iterator) {
return false;
}

// 4. If callback is undefined, set callback to
// 2. If callback is undefined, set callback to
// finalizationRegistry.[[CleanupCallback]].
RootedValue callback(cx);
if (callbackArg) {
Expand All @@ -796,193 +744,34 @@ bool FinalizationRegistryObject::cleanupQueuedRecords(
callback.setObject(*cleanupCallback);
}

// 5. Set finalizationRegistry.[[IsFinalizationRegistryCleanupJobActive]] to
// true.
registry->setCleanupJobActive(true);

FinalizationRecordVector* records = registry->recordsToBeCleanedUp();
#ifdef DEBUG
size_t initialLength = records->length();
#endif

// 6. Let result be Call(callback, undefined, iterator).
RootedValue iteratorVal(cx, ObjectValue(*iterator));
RootedValue rval(cx);
bool ok = Call(cx, callback, UndefinedHandleValue, iteratorVal, &rval);

// Remove records that were iterated over.
size_t index = iterator->index();
MOZ_ASSERT(index <= records->length());
MOZ_ASSERT(initialLength <= records->length());
if (index > 0) {
records->erase(records->begin(), records->begin() + index);
}

// 7. Set finalizationRegistry.[[IsFinalizationRegistryCleanupJobActive]] to
// false.
registry->setCleanupJobActive(false);

// 8. Set iterator.[[FinalizationRegistry]] to empty.
iterator->clearFinalizationRegistry();

return ok;
}

///////////////////////////////////////////////////////////////////////////
// FinalizationIteratorObject

const JSClass FinalizationIteratorObject::class_ = {
"FinalizationRegistryCleanupIterator",
JSCLASS_HAS_RESERVED_SLOTS(SlotCount), JS_NULL_CLASS_OPS,
JS_NULL_CLASS_SPEC};

const JSFunctionSpec FinalizationIteratorObject::methods_[] = {
JS_FN(js_next_str, next, 0, 0), JS_FS_END};

const JSPropertySpec FinalizationIteratorObject::properties_[] = {
JS_STRING_SYM_PS(toStringTag, "FinalizationRegistry Cleanup Iterator",
JSPROP_READONLY),
JS_PS_END};

/* static */
bool GlobalObject::initFinalizationIteratorProto(JSContext* cx,
Handle<GlobalObject*> global) {
Rooted<JSObject*> base(
cx, GlobalObject::getOrCreateIteratorPrototype(cx, global));
if (!base) {
return false;
}
RootedPlainObject proto(
cx, GlobalObject::createBlankPrototypeInheriting<PlainObject>(cx, base));
if (!proto) {
return false;
}
if (!JS_DefineFunctions(cx, proto, FinalizationIteratorObject::methods_) ||
!JS_DefineProperties(cx, proto,
FinalizationIteratorObject::properties_)) {
return false;
}
global->setReservedSlot(FINALIZATION_ITERATOR_PROTO, ObjectValue(*proto));
return true;
}

/* static */ FinalizationIteratorObject* FinalizationIteratorObject::create(
JSContext* cx, HandleFinalizationRegistryObject registry) {
MOZ_ASSERT(registry);

RootedObject proto(cx, GlobalObject::getOrCreateFinalizationIteratorPrototype(
cx, cx->global()));
if (!proto) {
return nullptr;
}

FinalizationIteratorObject* iterator =
NewObjectWithGivenProto<FinalizationIteratorObject>(cx, proto);
if (!iterator) {
return nullptr;
}

iterator->initReservedSlot(FinalizationRegistrySlot, ObjectValue(*registry));
iterator->initReservedSlot(IndexSlot, Int32Value(0));

return iterator;
}

FinalizationRegistryObject* FinalizationIteratorObject::finalizationRegistry()
const {
Value value = getReservedSlot(FinalizationRegistrySlot);
if (value.isUndefined()) {
return nullptr;
}
return &value.toObject().as<FinalizationRegistryObject>();
}

size_t FinalizationIteratorObject::index() const {
int32_t i = getReservedSlot(IndexSlot).toInt32();
MOZ_ASSERT(i >= 0);
return size_t(i);
}

void FinalizationIteratorObject::setIndex(size_t i) {
MOZ_ASSERT(i <= INT32_MAX);
setReservedSlot(IndexSlot, Int32Value(int32_t(i)));
}

void FinalizationIteratorObject::clearFinalizationRegistry() {
MOZ_ASSERT(finalizationRegistry());
setReservedSlot(FinalizationRegistrySlot, UndefinedValue());
}

// %FinalizationRegistryCleanupIteratorPrototype%.next()
// https://tc39.es/proposal-weakrefs/#sec-%finalizationregistrycleanupiterator%.next
/* static */
bool FinalizationIteratorObject::next(JSContext* cx, unsigned argc, Value* vp) {
CallArgs args = CallArgsFromVp(argc, vp);

// 1. Let iterator be the this value.
// 2. If Type(iterator) is not Object, throw a TypeError exception.
// 3. If iterator does not have a [[FinalizationRegistry]] internal slot,
// throw a TypeError exception.
if (!args.thisv().isObject() ||
!args.thisv().toObject().is<FinalizationIteratorObject>()) {
JS_ReportErrorNumberASCII(
cx, GetErrorMessage, nullptr, JSMSG_NOT_A_FINALIZATION_ITERATOR,
"Receiver of FinalizationRegistryCleanupIterator.next call");
return false;
}

RootedFinalizationIteratorObject iterator(
cx, &args.thisv().toObject().as<FinalizationIteratorObject>());

// 4. If iterator.[[FinalizationRegistry]] is empty, throw a TypeError
// exception.
// 5. Let finalizationRegistry be iterator.[[FinalizationRegistry]].
RootedFinalizationRegistryObject registry(cx,
iterator->finalizationRegistry());
if (!registry) {
JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
JSMSG_STALE_FINALIZATION_REGISTRY_ITERATOR);
return false;
}

// 8. If finalizationRegistry.[[Cells]] contains a Record cell such that
// cell.[[Target]] is empty,
// 3. While finalizationRegistry.[[Cells]] contains a Record cell such that
// cell.[[WeakRefTarget]] is empty, then an implementation may perform the
// following steps,
// a. Choose any such cell.
// b. Remove cell from finalizationRegistry.[[Cells]].
// c. Return CreateIterResultObject(cell.[[HeldValue]], false).
FinalizationRecordVector* records = registry->recordsToBeCleanedUp();
size_t index = iterator->index();
MOZ_ASSERT(index <= records->length());
// c. Perform ? Call(callback, undefined, « cell.[[HeldValue]] »).

// Advance until we find a record that hasn't been unregistered.
while (index < records->length() && index < INT32_MAX &&
!(*records)[index]->isActive()) {
index++;
iterator->setIndex(index);
}
RootedValue heldValue(cx);
RootedValue rval(cx);
FinalizationRecordVector* records = registry->recordsToBeCleanedUp();
FinalizationRecordSet* activeRecords = registry->activeRecords();
while (!records->empty()) {
FinalizationRecordObject* record = records->popCopy();

if (index < records->length() && index < INT32_MAX) {
RootedFinalizationRecordObject record(cx, (*records)[index]);
RootedValue heldValue(cx, record->heldValue());
PlainObject* result = CreateIterResultObject(cx, heldValue, false);
if (!result) {
return false;
// Skip over records that have been unregistered.
if (!record->isActive()) {
continue;
}

registry->activeRecords()->remove(record);
record->clear();
iterator->setIndex(index + 1);
heldValue.set(record->heldValue());

args.rval().setObject(*result);
return true;
}
activeRecords->remove(record);
record->clear();

// 9. Otherwise, return CreateIterResultObject(undefined, true).
PlainObject* result = CreateIterResultObject(cx, UndefinedHandleValue, true);
if (!result) {
return false;
if (!Call(cx, callback, UndefinedHandleValue, heldValue, &rval)) {
return false;
}
}

args.rval().setObject(*result);
return true;
}
6 changes: 0 additions & 6 deletions js/src/builtin/FinalizationRegistryObject.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,6 @@ class FinalizationRegistryObject : public NativeObject {
ActiveRecords,
RecordsToBeCleanedUpSlot,
IsQueuedForCleanupSlot,
IsCleanupJobActiveSlot,
SlotCount
};

Expand All @@ -194,11 +193,9 @@ class FinalizationRegistryObject : public NativeObject {
FinalizationRecordSet* activeRecords() const;
FinalizationRecordVector* recordsToBeCleanedUp() const;
bool isQueuedForCleanup() const;
bool isCleanupJobActive() const;

void queueRecordToBeCleanedUp(FinalizationRecordObject* record);
void setQueuedForCleanup(bool value);
void setCleanupJobActive(bool value);

void sweep();

Expand Down Expand Up @@ -227,9 +224,6 @@ class FinalizationRegistryObject : public NativeObject {

static void trace(JSTracer* trc, JSObject* obj);
static void finalize(JSFreeOp* fop, JSObject* obj);

static bool hasRegisteredRecordsToBeCleanedUp(
HandleFinalizationRegistryObject registry);
};

// An iterator over a finalization registry's queued held values. In the spec
Expand Down
4 changes: 2 additions & 2 deletions js/src/jit-test/tests/gc/bug-1620196.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

gczeal(4);
let heldValues = [];
registry = new FinalizationRegistry(iterator => {
heldValues.push(...iterator);
registry = new FinalizationRegistry(value => {
heldValues.push(value);
});
registry.register({}, 42);
gc();
8 changes: 4 additions & 4 deletions js/src/jit-test/tests/gc/bug1600488-1.js
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
// |jit-test| --enable-weak-refs

const token = {};
let iterated;
const finalizationRegistry = new FinalizationRegistry(items => {
iterated = items.next().value;
let cleanedUpValue;
const finalizationRegistry = new FinalizationRegistry(value => {
cleanedUpValue = value;
});
{
let object = {};
Expand All @@ -12,5 +12,5 @@ const finalizationRegistry = new FinalizationRegistry(items => {
}
gc();
finalizationRegistry.cleanupSome();
assertEq(iterated, token);
assertEq(cleanedUpValue, token);
assertEq(finalizationRegistry.unregister(token), false);
Loading

0 comments on commit 3a4567b

Please sign in to comment.