Skip to content

Clean up functions when disposed#227

Merged
justjake merged 15 commits intomainfrom
jake--functionid-freelist
Feb 16, 2026
Merged

Clean up functions when disposed#227
justjake merged 15 commits intomainfrom
jake--functionid-freelist

Conversation

@justjake
Copy link
Owner

@justjake justjake commented Jun 22, 2025

  • Re-works function binding in newFunction to use a different proxying strategy based on a new abstraction, HostRef. HostRef allows tracking references of host values from guest handles. Once all references to the HostRef object are disposed (either in the host, or GC'd in the guest), the host value will be dereferenced and become garbage collectable.
  • Functions passed to newFunction are interned in the runtime's HostRefMap until dereferenced.
  • HostRef is also exposed directly for use from QuickJSContext, see newHostRef, toHostRef, unwrapHostRef in the docs.
  • Added QuickJSContext.newConstructorFunction, such functions will have their this set to the newly constructed object, although they may also return a new object.

Fixes #223

@justjake justjake requested a review from Copilot June 22, 2025 22:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR cleans up function entries upon disposal by removing them from the internal map and reusing freed function IDs via a freelist instead of always incrementing the counter.

  • Extended heapValueHandle to accept an extra disposal callback.
  • Updated newFunction to pull IDs from fnIdFreelist and schedule freeFunction on handle disposal.
  • Introduced fnIdFreelist storage and freeFunction method for recycling IDs.
Comments suppressed due to low confidence (2)

packages/quickjs-emscripten-core/src/context.ts:1331

  • Add a JSDoc comment for fnIdFreelist to explain its role in recycling function IDs upon disposal.
  protected fnIdFreelist: number[] = []

packages/quickjs-emscripten-core/src/context.ts:608

  • Add unit tests to verify that IDs from fnIdFreelist are correctly reused when disposing and creating new functions.
    const fnId = this.fnIdFreelist.pop() ?? ++this.fnNextId

}

protected freeFunction(fn_id: number) {
const map_id = fn_id >> 8
Copy link

Copilot AI Jun 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extract the magic number 8 used in the bit shift into a named constant (e.g., ID_MAP_SHIFT_BITS) to clarify intent.

Suggested change
const map_id = fn_id >> 8
const map_id = fn_id >> QuickJSContext.ID_MAP_SHIFT_BITS

Copilot uses AI. Check for mistakes.
protected freeFunction(fn_id: number) {
const map_id = fn_id >> 8
const fnMap = this.fnMaps.get(map_id)
if (!fnMap) {
Copy link

Copilot AI Jun 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After deleting the last function in a map, consider removing the empty fnMap entry from fnMaps to avoid retaining unused maps.

Copilot uses AI. Check for mistakes.
@justjake justjake force-pushed the jake--functionid-freelist branch from a0e1f38 to 17a585b Compare February 15, 2026 21:56
@justjake justjake force-pushed the jake--functionid-freelist branch from d8d5c91 to f12b5e9 Compare February 16, 2026 03:24
@justjake justjake enabled auto-merge (squash) February 16, 2026 03:43
@justjake justjake merged commit fd9d3f0 into main Feb 16, 2026
1 check passed
@justjake justjake deleted the jake--functionid-freelist branch February 16, 2026 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integer overflow in function IDs

2 participants