On Sat, 14 Feb 2026 14:04:08 GMT, Michael Strauß <[email protected]> wrote:
>> Color.web(string, double) parses a color string by creating substrings of
>> the input. These string allocations can be removed.
>>
>> There are no new tests for the `Color` class, since the existing tests
>> already cover all relevant code paths.
>
> Michael Strauß has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review comment
modules/javafx.graphics/src/main/java/com/sun/javafx/css/parser/CssNumberParser.java
line 34:
> 32: private CssNumberParser() {}
> 33:
> 34: // Up to 19 decimal digits fit in an unsigned 64-bit long
do we need to parse all 19 digits? An IEEE 64-bit double has 52 bits of
mantissa, which translates into 2^53 = 9007199254740992 (length=16)
modules/javafx.graphics/src/main/java/com/sun/javafx/css/parser/CssNumberParser.java
line 211:
> 209: *
> 210: * @see <a href="https://arxiv.org/pdf/2101.11408";>Number Parsing at
> a Gigabyte per Second</a>
> 211: * @see <a href="https://arxiv.org/pdf/2212.06644";>Fast Number
> Parsing Without Fallback</a>
First of all, I am not against writing more efficient or allocation-free code
here.
I don't know whether CSS parsing is a frequent operation (it might be a
frequent operation to _apply_ the styles once the stylesheet got parsed, but
parsing itself should be run only when the stylesheet(s) have changed) - quite
possibly this change will produce a smoother app, I just don't have the data.
My concern is that we are bringing some cutting edge code for `Color.web()`
specifically, rather than rely on the JDK. Perhaps the optimization should be
in the JDK in `Double.parseDouble()`?
On the other hand, we can be the trailblazers. Did you write this code or was
it copied from some place? In other words, is it compatible with the license?
The second concern is testing. The new code is beyond what I consider a
reasonable amount of time and effort that I can allocate to the code review. I
mean, if we decide it is worth it, we can try, but I'd like @kevinrushforth to
get involved.
The main claim is that the this code is indistinguishable from
`Double.parseDouble()`, something that needs to be demonstrated. May be it'll
be sufficient to add more patterns (with `Float`s, we could test the whole
range, but it is unfeasible with 64-bit `Double`), especially near boundary
points.
What do you think?
modules/javafx.graphics/src/main/java/com/sun/javafx/css/parser/CssNumberParser.java
line 303:
> 301: * See "Table Generation Script" in "Number Parsing at a Gigabyte
> per Second" (Lemire, 2021), p. 32
> 302: *
> 303: * @see <a href="https://arxiv.org/pdf/2101.11408";>Number Parsing at
> a Gigabyte per Second</a>
is it really worth to go to all that trouble?
Does the CssParser re-parses the stylesheet every time a Node changes, or does
it parse the stylesheet(s) once and then works with the internal objects to
apply the styles?
modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/parser/CssNumberParserTest.java
line 168:
> 166: public void roundingIsCorrectFor64BitSignificands() {
> 167: int seed = new Random().nextInt();
> 168: System.out.println("Testing
> CssNumberParserTest.roundingIsCorrectFor64BitSignificands with seed " + seed);
btw, the seed should be `long`
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2069#discussion_r2818425224
PR Review Comment: https://git.openjdk.org/jfx/pull/2069#discussion_r2818584850
PR Review Comment: https://git.openjdk.org/jfx/pull/2069#discussion_r2818630658
PR Review Comment: https://git.openjdk.org/jfx/pull/2069#discussion_r2818509579
