Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8196586: Remove use of deprecated finalize methods from javafx property objects #113

Closed

Conversation

@kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Feb 13, 2020

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 WeakReferences 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.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8196586: Remove use of deprecated finalize methods from javafx property objects

Reviewers

  • Nir Lisker (nlisker - Committer)
  • Ambarish Rapte (arapte - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/113/head:pull/113
$ git checkout pull/113

@kevinrushforth kevinrushforth self-assigned this Feb 13, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 13, 2020

👋 Welcome back kcr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Feb 13, 2020
@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Feb 13, 2020

/reviewers 2

@openjdk
Copy link

@openjdk openjdk bot commented Feb 13, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Feb 13, 2020

@arapte I request you to be one of the reviewers of this PR.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 13, 2020

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Feb 14, 2020

I'll be the 2nd reviewer.

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Feb 16, 2020

the referent to the created property object has been garbage collected before the finalize method is called

I'm confused. What is that referent exactly and why is it guaranteed to have been GC'd before finalization?

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Feb 18, 2020

the referent to the created property object has been garbage collected before the finalize method is called

I'm confused. What is that referent exactly and why is it guaranteed to have been GC'd before finalization?

I meant the referent of the WeakReference to the constructed Property object on which the finalize method is being called.

The asObject and xxxxxProperty methods create a bidirectional binding, which uses weak references to both the original Property object and the newly constructed Property object. The finalize method is on the newly-constructed Property. By the time that finalize method is called, the JVM guarantees that no other reference to that Property object exists. From the API docs of Object::finalize:

The general contract of finalize is that it is invoked if and when the Java™ virtual machine has determined that there is no longer any means by which this object can be accessed by any thread that has not yet died, except as a result of an action taken by the finalization of some other object or class which is ready to be finalized.

@nlisker
Copy link
Collaborator

@nlisker nlisker commented Feb 20, 2020

The logic seems correct. The weak reference to the newly created property in the bidirectional binding must be cleared before finalize is called. The unbinding attempt only interferes with GC.

I still have to go over the tests.

I note that with the removal of the finalize methods, the method BidirectionalBinding#unbindNumber is left unused and can be removed.
Also, the changed property classes and BidirectionalBinding can use some cleanup that I can do after this is fixed, so I can remove that method at that time if you don't want to do it here.

@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Feb 20, 2020

I note that with the removal of the finalize methods, the method BidirectionalBinding#unbindNumber is left unused and can be removed.

Good catch. I'll remove that method as part of this fix, since it is directly related. Any other cleanup can be done later.

@arapte
arapte approved these changes Feb 27, 2020
Copy link
Member

@arapte arapte left a comment

Looks good to me too.

@openjdk
Copy link

@openjdk openjdk bot commented Feb 27, 2020

@kevinrushforth This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8196586: Remove use of deprecated finalize methods from javafx property objects

Reviewed-by: nlisker, arapte
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 8 commits pushed to the master branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate ef2f9ce96e21ff5ec4f528fb8e5f191632506791.

➡️ To integrate this PR with the above commit message, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Feb 27, 2020
@kevinrushforth
Copy link
Member Author

@kevinrushforth kevinrushforth commented Feb 27, 2020

/integrate

@openjdk openjdk bot closed this Feb 27, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Feb 27, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Feb 27, 2020

@kevinrushforth The following commits have been pushed to master since your change was applied:

  • ef2f9ce: 8238755: allow to create static lib for javafx.media on linux
  • c3ee1a3: 8239822: Intermittent unit test failures in RegionCSSTest
  • 3150562: 8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface
  • 4eaff0d: 8239109: Update SQLite to version 3.31.1
  • d8e7f85: 8239454: LLIntData : invalid opcode returned for 16 and 32 bit wide instructions
  • 48ddd80: Merge
  • 35a01ca: 8228867: Fix mistakes in FX API docs
  • b5e65ec: 8238434: Ensemble: Update version of Lucene to 7.7.2

Your commit was automatically rebased without conflicts.

Pushed as commit 9cd6f79.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 27, 2020

Mailing list message from Kevin Rushforth on openjfx-dev:

Changeset: 9cd6f79
Author: Kevin Rushforth <kcr at openjdk.org>
Date: 2020-02-27 12:14:17 +0000
URL: https://git.openjdk.java.net/jfx/commit/9cd6f791

8196586: Remove use of deprecated finalize methods from javafx property objects

Reviewed-by: nlisker, arapte

! modules/javafx.base/src/main/java/com/sun/javafx/binding/BidirectionalBinding.java
! modules/javafx.base/src/main/java/javafx/beans/property/BooleanProperty.java
! modules/javafx.base/src/main/java/javafx/beans/property/DoubleProperty.java
! modules/javafx.base/src/main/java/javafx/beans/property/FloatProperty.java
! modules/javafx.base/src/main/java/javafx/beans/property/IntegerProperty.java
! modules/javafx.base/src/main/java/javafx/beans/property/LongProperty.java
+ modules/javafx.base/src/test/java/test/javafx/beans/property/ObjectPropertyLeakTest.java

@kevinrushforth kevinrushforth deleted the kevinrushforth:8196586-rm-finalize-base branch Feb 27, 2020
@nlisker nlisker mentioned this pull request Mar 6, 2020
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.