[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: [Rev 03] RFR: 8196586: Remove use of deprecated finalize methods from javafx property objects
From:       Ambarish Rapte <arapte () openjdk ! java ! net>
Date:       2020-02-27 7:10:16
Message-ID: 1NtK_AcbdPHKXTW2NmP2ZtItcg4hj1lLCtECv2DH6Lo=.a339e333-db3e-45ed-8db4-e5c76f84cb45 () github ! com
[Download RAW message or body]

On Thu, 20 Feb 2020 20:43:22 GMT, Kevin Rushforth <kcr@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


[prev in list] [next in list] [prev in thread] [next in thread]