👋 Welcome back jhendrikx! 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.

Webrevs

• 12: Full - Incremental (cda93e50)
• 11: Full (d72aefab)
• 10: Full (0cf8eeb4)
• 09: Full - Incremental (95086e90)
• 08: Full - Incremental (e4e3a7ce)
• 07: Full - Incremental (2392d8db)
• 06: Full - Incremental (16da6eb0)
• 05: Full - Incremental (dc59c4db)
• 04: Full - Incremental (c388f23d)
• 03: Full (a33f797a)
• 02: Full - Incremental (5201cd2d)
• 01: Full - Incremental (6c3271b0)
• 00: Full (8da98928)

As @nlisker notes, the implications of any deprecation will need to be discussed. In particular, we need to consider what the consequences are for existing applications. If any of them might be used by apps, then simple deprecation (rather than deprecating for removal) might be best.

/reviewers 3
/csr

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 1 Reviewer, 2 Authors).

@kevinrushforth has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@hjohn please create a CSR request for issue JDK-8322964 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

As @nlisker notes, the implications of any deprecation will need to be discussed. In particular, we need to consider what the consequences are for existing applications. If any of them might be used by apps, then simple deprecation (rather than deprecating for removal) might be best.

The methods cannot be reached easily even though they are public. It would involve creating a Selector using Selector#createSelector, and then casting it to SimpleSelector to get access to its public methods that are not inherited from Selector. SimpleSelector itself does not have a public constructor, and the type SimpleSelector is not returned anywhere directly (it is always just Selector in the public API).

The only use of one of the deprecated methods is to allow access for the internal class SelectorPartitioning which indeed does an instanceof to reach it. The other method is not used anywhere, and seems to have been an attempt to make the selectors available in the same form as you'd find them on nodes Styleable#getStyleClass (which is a List), but is hard to reach (probably an oversight that it got exposed in the final API).

Google searches turn up no hits in 3rd party code for either of these.

I've posted on the mailing list to discuss it further.

Mailing list message from Dirk Lemmermann on openjfx-dev:

I truly love the fact that you guys are starting to use JFXCentral for benchmarking / testing :-) If anyone encounters things bad for performance unrelated to JavaFX itself but caused by JFXCentral then please let me know.

Dirk

Mailing list message from John Hendrikx on openjfx-dev:

It's a very nice application, I had never seen it before.? As the
original bug was reported against it that caused a performance
regression, I started using it to also test a potential performance
improvement.? Although I think it does a back-end call every time I
"refresh", so I might have been hitting your servers a bit during that
period.

I'll let you know if I discover that's off.? Only thing I did was
disable a piece of code that was causing a stack trace to be logged each
time (a bug in one of the components, fx tray icon, I think).

--John

On 04/01/2024 22:49, Dirk Lemmermann wrote:

I truly love the fact that you guys are starting to use JFXCentral for benchmarking / testing :-) If anyone encounters things bad for performance unrelated to JavaFX itself but caused by JFXCentral then please let me know.

Dirk

@hjohn This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Code looks good to me. As mentioned above, I tested it already and everything was good, so I'm certain that there is no regression here.

I'm generally not a big fan of 'reimplementing' Collections, but I can see why it is needed here and it also makes sense, especially for something like CSS which needs to be fast. Something I saw as well in Swing (just check the ArrayTable class 😄 ).

I'm not a fan either, especially because custom collection implementations when referred to by their interface may sneak their way through the various classes and eventually be returned to users. Any deviation from the collection contracts then suddenly is a bug users may encounter. I would have done this with plain HashSet (or Set.of) were it not for the fact that a lot of performance gains were possible due to the limited way FX is using these sets.

I wonder if we may want to add some tests for the FixedCapacitySet?

Yeah, now that it is more likely that this will make it into FX, I will add a small set of unit tests for this class.

I wonder if we may want to add some tests for the FixedCapacitySet?

Yeah, now that it is more likely that this will make it into FX, I will add a small set of unit tests for this class.

Since this PR is ready to integrate, I think it would be fine to file a new test bug for the additional tests if you like. If you prefer to add the new tests now, that's fine, too (we can re-review it).

I wonder if we may want to add some tests for the FixedCapacitySet?

Yeah, now that it is more likely that this will make it into FX, I will add a small set of unit tests for this class.

Since this PR is ready to integrate, I think it would be fine to file a new test bug for the additional tests if you like. If you prefer to add the new tests now, that's fine, too (we can re-review it).

I'm fine integrating this as-is and adding a test soon after. I will leave this over the weekend to give others time to review.

Also some clarification on the contributing rules: "all Reviewers who have requested the chance to review have done so" -- does the indication at the top right of the PR count towards this or should it be a comment? :) In the first case, @nlisker and @arapte, please indicate if you wish to review this still.

The code looks good. I didn't test it, but I'm fine with integrating.

I built it and tested on a retail self-checkout app that uses a lot of css. All good.

Also some clarification on the contributing rules: "all Reviewers who have requested the chance to review have done so" -- does the indication at the top right of the PR count towards this or should it be a comment? :) In the first case, @nlisker and @arapte, please indicate if you wish to review this still.

If someone wants you to wait for them, they should make it clear by adding a comment. Also if someone has given substantive feedback, but hasn't (re)approved, it's good to give them a change to review.

@arapte can add a comment if he wants to review, otherwise go ahead and integrate on Monday.

@kevinrushforth @andy-goryachev-oracle @Maran23 @mstr2

I couldn't leave this without a proper test, so I created a test for FixedCapacitySet.

I didn't discover any issues, but did discover there was a line of code I couldn't cover in the OpenAddressed variant (not even with a specially crafted test case). I checked the code, and realized the situation can never occur, so I've removed those lines and added a comment and assert instead.

/integrate

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

• 2e1427e: 8087444: CornerRadii with different horizontal and vertical values treated as uniform
• dedcf1d: 8332539: Update libxml2 to 2.12.7

Your commit was automatically rebased without conflicts.

@hjohn Pushed as commit b08d7bb.

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