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

8273138: BidirectionalBinding fails to observe changes of invalid properties #614

Closed
wants to merge 2 commits into from

Conversation

mstr2
Copy link
Collaborator

@mstr2 mstr2 commented Aug 29, 2021

This PR fixes a bug that was introduced in #454.

Since this fix might impact the performance considerations in the original PR, I ran a performance benchmark against the previous ChangeListener-based implementation, which still shows better performance for the new implementation:

@State(Scope.Benchmark)
public class BindingBenchmark {
    DoubleProperty property1 = new SimpleDoubleProperty();
    DoubleProperty property2 = new SimpleDoubleProperty();

    public BindingBenchmark() {
        property2.bindBidirectional(property1);
    }

    @Benchmark
    public void benchmark() {
        for (int i = 0; i < 10000000; ++i) {
            property1.set((i % 2 == 0) ? 12345.0 : 54321.0);
        }
    }
}
Benchmark Mode Cnt Score Error Units
ChangeListener thrpt 5 7.463 0.040 ops/s
InvalidationListener (fixed) thrpt 5 15.095 0.092 ops/s

Progress

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

Issue

  • JDK-8273138: BidirectionalBinding fails to observe changes of invalid properties

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/614/head:pull/614
$ git checkout pull/614

Update a local copy of the PR:
$ git checkout pull/614
$ git pull https://git.openjdk.java.net/jfx pull/614/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 614

View PR using the GUI difftool:
$ git pr show -t 614

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/614.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 29, 2021

👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@mstr2
Copy link
Collaborator Author

mstr2 commented Aug 29, 2021

@kevinrushforth Should this be a new JBS ticket, or should we re-open 8264770 and associate this PR with it?

@hjohn
Copy link
Collaborator

hjohn commented Aug 29, 2021

This change looks good to me.

@kevinrushforth
Copy link
Member

kevinrushforth commented Aug 30, 2021

Should this be a new JBS ticket, or should we re-open 8264770 and associate this PR with it?

The former. We never reopen a bug that was resolved/fixed by a commit in the repo.

@mstr2 mstr2 changed the title BidirectionalBinding fails to observe changes of invalid properties 8273138: BidirectionalBinding fails to observe changes of invalid properties Aug 30, 2021
@openjdk openjdk bot added the rfr Ready for review label Aug 30, 2021
@mlbridge
Copy link

mlbridge bot commented Aug 30, 2021

Webrevs

@kevinrushforth
Copy link
Member

kevinrushforth commented Aug 30, 2021

/reviewers 2

@kevinrushforth kevinrushforth self-requested a review Aug 30, 2021
@openjdk
Copy link

openjdk bot commented Aug 30, 2021

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

Copy link
Member

@kevinrushforth kevinrushforth left a comment

The explanation makes sense, and both the fix and the test look good. I can confirm that the new test fails without the fix and passes with the fix.

@kevinrushforth kevinrushforth requested a review from arapte Sep 4, 2021
@fthevenet
Copy link
Member

fthevenet commented Sep 6, 2021

Am I correct in assuming that, as is, this PR is only going to be merged into openJFX 18 ?
If so, because it addresses a regression introduced in jfx 17, we'll probably want to have this one in 17 as well on day one, if its not too late.

@mstr2
Copy link
Collaborator Author

mstr2 commented Sep 6, 2021

It seems to be too late.

@fthevenet
Copy link
Member

fthevenet commented Sep 6, 2021

I see. This is unfortunate though, as this will definitely break some applications in ways potentially difficult to diagnose.

Maybe this won't be too bad if the window until 17.0.1 is small, but still, I'm afraid third party developers not following these discussions might be in for some head scratching.

@kevinrushforth
Copy link
Member

kevinrushforth commented Sep 6, 2021

Yes, it is far too late for 17 (code freeze was more than two weeks ago; GA is tomorrow).

17.0.1 should be out in about 6 weeks, and this is a candidate for backporting.

@fthevenet
Copy link
Member

fthevenet commented Sep 6, 2021

OK I see. I didn't realize GA is due for tomorrow already.
I had September 14 in mind as a release date, but realize now that this is the date for OpenJDK.

@nlisker
Copy link
Collaborator

nlisker commented Sep 7, 2021

There's an ongoing discussion about the eager evaluation of invalidation listeners in the mailing list. Its conclusion might affect this patch, I didn't look at this one closely.

@mstr2
Copy link
Collaborator Author

mstr2 commented Sep 7, 2021

There's an ongoing discussion about the eager evaluation of invalidation listeners in the mailing list. Its conclusion might affect this patch, I didn't look at this one closely.

The discussion is about whether adding a listener should validate the property. With this fix, both properties are validated in all relevant code paths, so it doesn't matter whether or not any of the properties is validated by adding an invalidation listener.

@kevinrushforth
Copy link
Member

kevinrushforth commented Sep 7, 2021

I think that restoring the existing behavior to fix this regression is important, and needs to be done regardless of what we might do in the future.

@mstr2
Copy link
Collaborator Author

mstr2 commented Sep 7, 2021

I think that restoring the existing behavior to fix this regression is important, and needs to be done regardless of what we might do in the future.

This PR does that.

@kevinrushforth
Copy link
Member

kevinrushforth commented Sep 7, 2021

I think that restoring the existing behavior to fix this regression is important, and needs to be done regardless of what we might do in the future.

This PR does that.

Indeed it does. My comment was meant in reply to the point @nlisker raised. Sorry for not making that clear.

arapte
arapte approved these changes Sep 8, 2021
Copy link
Member

@arapte arapte left a comment

looks good to me too.

@openjdk
Copy link

openjdk bot commented Sep 8, 2021

@mstr2 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8273138: BidirectionalBinding fails to observe changes of invalid properties

Reviewed-by: kcr, arapte

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 5 new commits pushed to the master branch:

  • 5552213: Merge
  • 24d0600: 8273343: Create release notes for JavaFX 17
  • 78ae4a8: 8269871: CellEditEvent: must not throw NPE in accessors
  • 2267b11: 8273071: SeparatorSkin: must remove child on dispose
  • e931501: 8269081: Tree/ListViewSkin: must remove flow on dispose

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @arapte) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Ready to be integrated label Sep 8, 2021
@mstr2
Copy link
Collaborator Author

mstr2 commented Sep 8, 2021

/integrate

@openjdk openjdk bot added the sponsor Ready to sponsor label Sep 8, 2021
@openjdk
Copy link

openjdk bot commented Sep 8, 2021

@mstr2
Your change (at version 5cdaa7e) is now ready to be sponsored by a Committer.

@nlisker
Copy link
Collaborator

nlisker commented Sep 8, 2021

/sponsor

@openjdk
Copy link

openjdk bot commented Sep 8, 2021

Going to push as commit 26d6438.
Since your change was applied there have been 5 commits pushed to the master branch:

  • 5552213: Merge
  • 24d0600: 8273343: Create release notes for JavaFX 17
  • 78ae4a8: 8269871: CellEditEvent: must not throw NPE in accessors
  • 2267b11: 8273071: SeparatorSkin: must remove child on dispose
  • e931501: 8269081: Tree/ListViewSkin: must remove flow on dispose

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 8, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Sep 8, 2021
@openjdk
Copy link

openjdk bot commented Sep 8, 2021

@nlisker @mstr2 Pushed as commit 26d6438.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
6 participants