8301283: Util methods for computing text/width height giving up some performance

[Maran23]

This one took me actually some hours to fully understand.
I recommend reading the ticket description made by Phil as it already explains most of it.

What is the problem (in Utils):

• Utils.computeTextWidth(..) does not initialize the boundsType. As a consequence, we do not cache the layout result
  • For the text width, it makes sense to set it, so the result is cached.
    Note that the boundsType does not matter for the width calculation itself, but we really want it set so the result will be cached if possible
• At one point due to Utils.computeTextHeight(..) method call, the boundsType is initialized. Now calls to Utils.computeTextWidth(..) do cache the result
  • So in order to fix that, Utils.computeTextWidth(..) does now set the the property as well

Ok, but why only cache this specific usecase (Phil was also wondering about that) (in PrismTextLayout):

• First of all, the previous method name was confusing (me).
  What it actually does:
  • decide if the layout result should be cached
  • when we have a cache hit, if we can reuse it completely or ONLY the text runs
• So what PrismTextLayout does is to cache the layout result for simple cases with the default setting.
  • This is biased towards modena, which specifies -fx-bounds-type: logical_vertical_center; for ever Text node
    • If one of those setting or the bounds type do not match, we will not cache the text layout or reuse it completely, but the text runs instead
• That means the logic is fine and there's a thought behind it: Only cache if the settings match those we are most likely to receive from the application
  • In case we have already a cache result but for example the wrapWidth or the boundsType is different, we can reuse the runs but not the whole cached layout result
  • So we should not cache results with boundsType = 0, as those will have another height than with boundsType = BOUNDS_CENTER
    • So the logic is unchanged there

As StubTextLayout extends PrismTextLayout, we can actually write meaningful tests:

• So I wrote a bunch that make clear how the cache works and when a GlyphLayout is created (due to a complete cache miss)
• Note that the tests are green before and after this PR
• Because PrismTextLayout.setContent(..) will request the font strike (for hashCode calculation for the layout result cache), I needed to implement hashCode and equals for some of the stub test classes (the JavaFX font classes do that as well)
• I renamed the StubFontContractTest to a better name since we do test a bit more now
  • And back in 2024 when I wrote this test class, I forgot the copyright header 🙃. Added now

Can we improve more?

• The ticket suggests that we could cache more in LabeledSkinBase. There are also some TODOs in this class. This could be a potential follow-up.

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8301283: Util methods for computing text/width height giving up some performance (Bug - P4)

Reviewers

• Andy Goryachev (@andy-goryachev-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2145/head:pull/2145
$ git checkout pull/2145

Update a local copy of the PR:
$ git checkout pull/2145
$ git pull https://git.openjdk.org/jfx.git pull/2145/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2145

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2145.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 01: Full - Incremental (8cc6d9c8)
• 00: Full (1baaa116)

[kevinrushforth]

Reviewer: @andy-goryachev-oracle

@prrace Would you also be able to review this? Or comment on the points raised in the PR description?

/reviewers 2

[openjdk]

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

[andy-goryachev-oracle]

No ill effects as far as i can see with the Monkey Tester on macOS 26.3.1

I do see build errors in Eclipse:

Description	Resource	Type	Path	Location
GlyphLayout cannot be resolved to a type	TextLayoutUtilsContractTest.java	Java Problem	/controls/src/test/java/test/javafx/scene/control/skin	line 245
GlyphLayout cannot be resolved to a type	TextLayoutUtilsContractTest.java	Java Problem	/controls/src/test/java/test/javafx/scene/control/skin	line 277
GlyphLayout cannot be resolved to a type	TextLayoutUtilsContractTest.java	Java Problem	/controls/src/test/java/test/javafx/scene/control/skin	line 310
GlyphLayout cannot be resolved to a type	TextLayoutUtilsContractTest.java	Java Problem	/controls/src/test/java/test/javafx/scene/control/skin	line 342
The type com.sun.javafx.text.GlyphLayout is not accessible	TextLayoutUtilsContractTest.java	Java Problem	/controls/src/test/java/test/javafx/scene/control/skin	line 30

(I've merged the latest master branch before testing)

[andy-goryachev-oracle]

Can you add this change to fix the Eclipse warnings:

https://github.com/andy-goryachev-oracle/jfx/blob/ag.2145/modules/javafx.controls/.classpath

[Maran23]

Can you add this change to fix the Eclipse warnings:

https://github.com/andy-goryachev-oracle/jfx/blob/ag.2145/modules/javafx.controls/.classpath

I don't feel to confident doing something like that, since I can not test it. Will just copy your code for now, but can not do any reasoning about it or fixes if someone git blame me in the future

[andy-goryachev-oracle]

Thanks!
All it does is adding <attribute name="add-exports" value="javafx.graphics/com.sun.javafx.text=javafx.controls"/> on L14

[andy-goryachev-oracle]

The changes look good!

The only product change is adding missing TextLayout configuration items in Utils.