On Mon Dec 5 15:03:29 2022 +0000, Jacek Caban wrote:
I also have an idea how to fix the scopes not being collected right
now, in `InterpretedFunction_gc_traverse`, which can then simplify it to a couple of lines. But it needs some changes to the scope_chain_t so that it is wrapped in a dummy jsdisp, so I'll keep that for another MR. I will add tests for that too, of course. Anyway just wanted to mention that the `FIXME` there is temporary! A dummy jsdisp sounds worrying and handling things like scopes is an important part of GC, so I'd rather wait and see the whole thing. I'd expect that abstracting "GC thing" instead of binding it so tight to jsdisp_t would be better, especially with Gecko CC integration on the horizon. See how `note_cc_edge()` works, for example. I'm also not sure if modifying object refs in GC and restoring them later is needed. You could, for example, store count of noted references to the object in GC and later compare object's ref count to noted ref count. If it's equal, you know you may unlink it. This would avoid modifying objects until we know if we want to unlink them or not.
The problem with the noted ref count approach is that I cannot efficiently or easily map between the object and its noted refcount, which is why I'm modifying it directly. In contrast, the saved refs don't have to be randomly accessed, only as we traverse the object list to restore them…
About abstracting the GC, I think that might add management overhead or code complexity. A dummy jsdisp isn't such a big deal, but I can include it in the MR, probably at the beginning of the series so that the GC is simplified to begin with.
It basically replaces the "LONG ref" field of the scope_chain_t with a `jsdisp_t` field, which provides the ref itself, and further provides the builtin vtbl that the GC will use. In the vtbl we will traverse the `scope_chain_t` properly. That's all.
Note that without scope chain handling it's still correct, it will just leak. The important and necessary part is to unlink all objects during unlinking, which is implemented even without the dummy jsdisp; it must make sure that releasing the object will not release any other object, and that's what happens when the object is completely unlinked from any other ref.
Failing to do the other traverse ops will just mean those objects don't get visited, which treats them as "external refs" and so will consider them immune from the gc process, hence leaking, but not crashing.
But if I move that patch to the beginning it wouldn't be a problem anymore of course. I will do it since it's simpler.
So to summarize:
1) I can't see a way to efficiently refcount the objects without modifying them, unless I store the gc refcount in the objects themselves as an extra field. This would somewhat simplify the `gc_run` function only, but it would waste memory on every single object.
There's an alternative here as well: I can traverse the objects and restore them only at the end, instead of during the second stage. This *somewhat* simplifies it, although I still have to use chunks to avoid allocating too much contiguous memory. Do you think this is enough?
2) I think abstracting the GC would lead to more hassle than it's worth, considering that it would only help for scope_chain_t and the fix is relatively simple now. I can add that to the beginning of the patch so you can see why it's pretty simple.
I'm not sure about (1) though.