On Thu, 20 Feb 2020 20:43:22 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> This patch removes the `finalize` methods from the `Property` classes in the
>> `javafx.base` module.
>>
>> The `{Boolean,Double,Float,Int,Long}Property` classes each have a pair of
>> methods -- `asObject` and `xxxxxProperty` -- that create a `Property<Xxxxx>`
>> object from a primitive `XxxxxProperty` object and vice versa. Each of the
>> methods bidirectionally binds the original property object to the created
>> property object. All of the bidirectional bindings in question use
>> `WeakReference`s to refer to the pair of objects being bound to each other,
>> so that they won't cause a memory leak.
>>
>> The `finalize` methods were added in an attempt to cleanup the bidirectional
>> bindings when the created object goes away. I say attempt, because it is
>> entirely ineffective in doing so. The logic that removes the binding creates
>> a new one from the same pair of objects, but fails to match the original
>> binding because the referent to the created property object has been garbage
>> collected before the `finalize` method is called; the `WeakReference` in the
>> binding is cleared and no longer matches. Fortunately, the only impact of
>> this ineffective logic is that it can delay the removal of the binding
>> (which is a small object that just contains the two weak references) from
>> the original property object until it (the original property) is either
>> garbage collected or modified (the binding logic already looks for a
>> referent that has been GCed and removes the binding).
>>
>> Given that the `finalize` methods don't do anything useful today, and that
>> there is no memory leak in the first place, the proposed fix is to just
>> remove the `finalize` methods entirely with no replacement needed. There
>> will be no changes in behavior as a result of this.
>>
>> Existing tests of the methods in question are sufficient to ensure no
>> functional regressions, although there is no existing test for leaks, so I
>> created new tests to verify that there is no leak. Since there is no
>> existing leak, those tests will pass both with and without this fix.
>
> The pull request has been updated with 1 additional commit.
Looks good to me too.
-------------
Marked as reviewed by arapte (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/113
