[Gc] Re: Patch for native support of long weakrefs
ivmai at mail.ru
Tue Nov 13 01:52:59 PST 2012
The general algorithm of the patch is ok. But I can't accept this patch without significant refactoring because:
* it contains both code refactoring and feature addition in a single patch
* the patch is too big for eye-inspection
* it contains big #defines (a 5-line define is ok but bigger is questionable) - complicates stepping thru the code in debugger as well as static code analysis
* code adding long-refs functionality should be put in some NOT_NEEDED ifdef (similar to GC_MOVE_DISAPPEARING_LINK_NOT_NEEDED or JAVA_FINALIZATION_NOT_NEEDED).
Could I ask you to perform some refactoring of existing code in finalize.c before adding new feature?
If yes, please follow these steps:
1. clone bdwgc repository at github (exactly this service is not strictly necessary but should simplify code reviewing)
2. checkout some new development branch (e.g., "add-long-ref")
3. Replace put dl_head, log_dl_table_size and dl_entries into single structure, define GC_dl_hashtbl global var instead of 3 ones (code refactoring, step 1)
4. Wait for code review results
5. Move most code of GC_general_register_disappearing_link (except for arguments checks and locks) to new GC_register_disappearing_link_inner (it could be GC_INLINE) which should accept pointer to GC_dl_hashtbl (in addition to link and obj);
same for GC_unregister_disappearing_link and GC_move_disappearing_link (code refactoring, step 2)
6. Wait for code review results and propose idea (without writing the code immediately) how to refactor GC_finalize to simplify adding long-ref functionality
Also, about "long ref" term: is there some better naming (as I understand this is close to Java Phantom refs but phantom might be not descriptive as well). Could you point me to the place from which you took "long ref" term? Any ideas for better naming?
And, please ask if something is unclear before writing significant amount of the code (there should be no rush).
Thank you. Sorry about loading with "extra" refactoring tasks before you get the requested functionality added to GC.
PS. Having added new functionality, then, next GC release, we could think about more efficient finalization/weak_refs implementation (preserving public API).
Mon, 12 Nov 2012 17:16:14 +1100 Zach Saw <zach.saw at gmail.com>:
> Hi Hans,
>>- The comment for GC_general_register_disappearing_link_ex (or preferably a separate replacement function as Ivan suggested) needs to be much more careful
in describing the functionality. I’m not sure I understand yet. You want to remove links if objects are no longer reachable from anything, including from finalizable objects? Isn’t that easier to determine after all finalizable objects have been traced?
>It turns out you're right. It is indeed as simple as that!
>Here's the simplified patch. Can't believe it's actually so simple to add long weakrefs support directly into the GC.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the Gc