Skip to content

Implement FinalizationRegistry and WeakRef #2593

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 14 commits into from

Conversation

bullno1
Copy link
Contributor

@bullno1 bullno1 commented Dec 20, 2022

Resolve #1299

All tests fail right now because fixtures are not recreated.
This is created to gather feedbacks.

One debug output is attached to see the impact of this change.
AFAIK, there is no impact on release builds when the feature is not used.

Current quirks of FinalizationRegistry:

  • register only takes 2 arguments and the 3rd argument is assumed to be the object.
    register always take 3 arguments because undefined is not representable.

  • Each object can only be registered once per registry.
    Simplify the implementation for what I believe is the most typical use case (see below).
    Each token can only be registered once.

  • Repeated register and unregister will consume extra memory until the target object is collected.
    This is because new book keeping entries are created in register but not removed in unregister.
    The reason is again, to keep the implementation simple and a typical use case should only unregister once, typically in an explicit "close" method (see below).

    Even in the case of duplicated entries, finalizer is still guaranteed to be called once per token.

declare function closeFd(fd: u32); void; // some native function

class Socket {
    private static FINALIZATION_REGISTRY = new FinalizationRegistry<u32>(closeFd);

    constructor(private fd: u32) {
        FINALIZATION_REGISTRY.register(this, fd, this);
    }

    close() { // explicit clean up
        if (FINALIZATION_REGISTRY.unregister(this)) {  // can call close multiple times safely
            closeFd(fd);
        }
    }
}
  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

@dcodeIO
Copy link
Member

dcodeIO commented Dec 20, 2022

Regarding making FRs GC-able, would something like this work?

let ptrToRegistry = new Map<usize,FinalizationRegistry[]>();

function finalizeImpl(ptr: usize): void {
  if (!ptrToRegistry.has(ptr)) return;
  let registries = ptrToRegistry.get(ptr);
  for (let i = 0, k = registries.length; i < k; ++i) {
    registries[i].finalize(ptr);
  }
  ptrToRegistry.delete(ptr);
}

so when finalizeImpl is invoked with a pointer, it can forward to the FRs handling the pointer, and afterwards delete the pointer so the respective FRs can be collected? Any registry's register would then register with ptrToRegistry.

@bullno1
Copy link
Contributor Author

bullno1 commented Dec 20, 2022

Yes, I think that's better.

@bullno1
Copy link
Contributor Author

bullno1 commented Dec 20, 2022

I guess my problem with this is that GC pressure is kind of amplified.
register creates an array.
__finalize drops the array for the next collection cycle.

It might be worth pooling those arrays.

@jtenner
Copy link
Contributor

jtenner commented Dec 21, 2022

Fwiw finalization was implemented using as-disposable already and it had to deal with quite a lot of problems

https://github.com/jtenner/as-disposable

@bullno1
Copy link
Contributor Author

bullno1 commented Dec 21, 2022

@dcodeIO There are still a lot of tests to write but this change will be huge due to the code change to GCs to support __finalize as an index.
Thankfully, it should only affect debug fixtures.
Should I make a separate PR for that?

I'd argue it's a better GC hook API than __finalize as a symbol.
It allows the stdlib hook to be improved without affecting fixtures.
Moreover, multiple hooks can be set at the same time by chaining to the previous hook.

However, it will break as-disposable or anything that relies on __finalize being an optional symbol.
It is possible to keep compatibility by putting something like this in GC initialization:

// now call it __finalize_index instead
export let __finalize_index: u32 = 0;

// When initializing GC
if (isDefined(__finalize)) {
    __finalize_index = __finalize.index; 
}

After the __finalize change, I guess I will only submit FinalizationRegistry first.
WeakRef and WeakMap have many little details.

@jtenner
Copy link
Contributor

jtenner commented Dec 21, 2022

I'm glad this is at least being talked about. Can we not break something that lunatic devs rely on please? It would be nice to rely on __finalize as a feature.

@github-actions
Copy link

This PR has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs. Thank you for your contributions!

@github-actions github-actions bot added the stale label Oct 18, 2023
@github-actions
Copy link

This PR has been automatically closed due to lack of recent activity, but feel free to reopen it as long as you merge in the main branch afterwards.

@github-actions github-actions bot closed this Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support WeakRef / FinalizationRegistry
3 participants