Share converted objects instead of nilling every repeat

[ursm]

Fixes #90.

The visited set added for cycle detection was never unwound, so it answered "have I ever seen this object?" rather than "is this object an ancestor of the one I am converting?". Every object reached twice became nil, whether or not a cycle was involved.

Quickjs.eval_code('const o = {a: 1}; [o, o]')            # before: [{"a"=>1}, nil]
Quickjs.eval_code('const o = {a: 1}; ({x: o, y: o})')    # before: {"x"=>{"a"=>1}, "y"=>nil}

Now both branches convert, and the two occurrences are the same Ruby object — which is what the guest built.

What changed

An object's address maps either to a marker meaning "still converting, so this occurrence is a cycle" or to the Ruby value it finished producing. One map serves both roles; using it as its own marker cannot collide with any value an object could convert to.

Cycles are unaffected — the existing tests from #33 pass untouched. Deep DAGs stop expanding: the 20-level example from the issue goes from losing every right-hand branch to converting in 0.2 ms with one hash per level.

Pinning. The map is keyed by raw addresses, and getters and toJSON run guest JS that can free an object and let a new one land on the same address. Every tracked object is pinned with JS_DupValue and released in an rb_ensure. This is load-bearing rather than defensive: removing the pin makes the new an object freed by a getter cannot be mistaken for an earlier one test fail with the recycled object's value.

toJSON results are not memoized. They are the object's stand-in rather than a container this conversion built. Sharing them handed both slots of [d, d] the same unfrozen String for a Date, where << on one rewrites the other. Recomputing per occurrence also matches JSON.stringify. When toJSON returns another object, that object's conversion is still shared — correctly, since the stand-in genuinely is that object.

Two properties worth knowing

Both are discussed at more length in the issue.

• One snapshot per object. A getter that mutates the graph mid-conversion is not observed, because the mutated object may already have converted. Observing it means converting each occurrence independently, which is the amplification the memo exists to prevent.
• A cycle's truncation point follows traversal order and is then shared by every occurrence of the truncated object. The repair — skipping the memo for subtrees that truncated — puts the exponential expansion one line of guest JS away (cur.self = cur), so the memo is unconditional and the asymmetry is pinned by a test instead.

Review

Two adversarial reviews, on memory safety and on behaviour. Findings applied here: toJSON results excluded from the memo, conv_pin publishing the new buffer before freeing the old one, the address-reuse regression test, and a misleading comment on the DAG test. The reviews found no GC-safety defect (ConvState is stack-resident, so conservative scanning pins it; verified under GC.compact from inside getters and GC.stress), no leak on the pin path, and proved the NULL-conv fast path cannot reach the object branch.

Pre-existing issues they turned up are filed separately: #98 (revoked Proxy converts to []) and #99 (conversion leaks JS objects when it raises — same walk, worth deciding alongside this).

Note on #81: releasing the pins issues refcount writes at the end of a conversion, so a dispose! from a getter now lands a few more writes in freed storage. The extra writes are new, the use-after-free is not, and #81's fix removes both.

Benchmark

100k conversions, median of three runs:

	before	after
primitive	77 ms	77 ms
small plain object	176 ms	184 ms
class instance	140 ms	147 ms
50-key object	132 ms	132 ms

A second measurement on larger payloads (20 × a 20k-element array of objects) came out at 0.292 s → 0.311 s, i.e. within the same range. The cost is the rb_ensure and one extra hash write per object; primitives skip the machinery entirely. Two separate maps cost twice as much, which is why there is one.

579 runs, 0 failures.

🤖 Generated with Claude Code

[hmsk]

Reviewed this properly rather than skimming it, because the memo changes what a converted value is and not only what it contains. It holds up. Notes below are one thing I would like added, one mechanical thing, and some corroboration for claims that were worth checking rather than taking on trust.

Verified independently

Built and probed on this branch, against main for the comparisons.

• [o, o] converts both slots and they are equal?. {x: o, y: o} likewise.
• Cycles are untouched: self reference, mutual reference and a self-containing array all still produce nil in the right place.
• The 20-level DAG from Cycle detection nils out non-cyclic shared references (DAGs), losing data #90 converts in 0.1ms with both branches present and shared, against losing every right branch before.
• toJSON is recomputed per occurrence: [d, d] for a Date gives two Strings that are not equal?, so the mutable-string trap you describe is closed.
• A real toJSON cycle, where the method lives on the prototype and returns {me: this}, gives {"me" => nil, "v" => 1}. Correct, and not the same code path as an own toJSON property, which stays a plain object.
• Re-entrancy works. A getter calling a define_function bridge that runs its own eval_code gives each conversion its own map: both share internally, neither shares with the other.

The claim about #99 is right, and worth having checked. Identical inputs, main against this branch:

returned value	main	this branch
({a: 1, p: Promise.resolve(1)})	2.00	2.00
({a: {b: 1}, p: Promise.resolve(1)})	3.00	3.00
[1, Promise.resolve(1)]	2.00	2.00

objects leaked per eval, 3,000 evals each. The pins are released on the raising path, so this adds nothing to that leak.

One thing I looked at and found fine: an object excluded from the memo pins on every occurrence, so new Array(100000).fill(sameDate) pins 100,000 times. That is bounded by the array it is already paying for, and it converted in 64ms.

One request: the README should say that occurrences are shared

This introduces an observable property that the type table does not cover. The same JS object reached twice is now the same Ruby object, so:

r = Quickjs.eval_code('const o = {a: 1}; [o, o]')
r[0]['a'] = 999
r  #=> [{"a" => 999}, {"a" => 999}]

Before this, nobody could depend on either behaviour, because the second occurrence was nil. After it, a caller mutating one branch of a result quietly mutates the others, and there is nothing in the documentation that would lead them to expect it. Your PR description and the issue both weigh this properly; it deserves to be somewhere a user will actually look. A line under Value Conversion, next to the undefined and NaN notes, seems like the right size.

The two properties in your description, one snapshot per object and the truncation point following traversal order, I would leave in the code and the issue where they are. They are reasoning about the implementation. Aliasing is something a caller can trip over.

Mechanical

Needs a rebase: this is on 1acbe9d, so it predates #95 and #102. #102 in particular replaced assert_run_in_parallel with assert_releases_gvl, so the merge is clean but the test file has moved underneath it.

On the tests

The ten cases cover this better than I expected, including the one I asked for on #90: an object that is both an ancestor and a share. an object freed by a getter cannot be mistaken for an earlier one is the one I would have been most worried about writing myself, and it is the reason the pin is load-bearing rather than defensive, which your description says and the test then proves.

[ursm]

Both addressed.

README. Added under the conversion table, in the shape you asked for — the aliasing a caller can trip over, with the mutation shown rather than described, plus one line that a cycle converts to nil where it closes. The two implementation properties stay in the issue and the code, which I agree is where they belong: a user does not need to know how the truncation point is chosen, only that mutating one branch changes the others.

Rebased onto 654acbc. Clean — my tests do not touch the parallel helpers, so #102 replacing assert_run_in_parallel with assert_releases_gvl moved the file underneath without conflicting. 601 runs, 0 failures on the new base, and I re-ran the sharing, cycle, DAG and toJSON probes afterwards rather than trusting the suite alone.

Thank you for checking the #99 claim with the same inputs on both sides. That was the one I most wanted a second measurement on: "the pins are released on the raising path" is easy to assert and hard to see, and identical per-eval counts on main and here is the evidence that the rb_ensure actually runs when the conversion unwinds.

The new Array(100000).fill(sameDate) case is a better probe than anything I ran — an object excluded from the memo pins per occurrence, and I had only reasoned that it was bounded rather than measuring it. Good to have 64ms attached to that.

Your prototype-toJSON cycle case is one I missed: the method living on the prototype takes the non-plain path while an own toJSON property leaves the object plain, so they are different branches that happen to look identical from JS. That distinction survives untested. I would rather it did not — say the word and I will add it here, or fold it into the #99 work, which touches the same walk.

[hmsk]

Flagging the sequencing here too, since it decides what happens next in both places: this should land before the #99 work, and it needs only the README line from my review above. I have asked on #99 to hold the follow-up until then rather than stacking on this branch.

To restate the one ask, so it is not buried: the same JS object reached twice now converts to the same Ruby object, so mutating one branch of a result mutates the others. That is worth a line under Value Conversion. A rebase onto main is also due, since this predates #95 and #102.

Everything else I checked held up, including the leak parity with main on the #99 path.

[hmsk]

Both addressed, and the README wording is better than what I asked for. Splitting it into its own commit was the right call, and "mirroring the graph JavaScript built" says why the sharing is correct rather than only that it happens, which is the part a reader needs to accept it as a feature instead of a leak. The cycle sentence after it closes the obvious next question.

Ran the README example verbatim to be sure the documentation is not aspirational:

equal?:  true
mutated: [{"a" => 999}, {"a" => 999}]
cycle:   {"self" => nil}

Rebase looks right, on 654acbc. 601 runs, 0 failures locally; 11 of 11 green on CI.

Nothing further from me.