8377427: Reduce substring allocations in Color.web(String, double) #2069

mstr2 · 2026-02-08T04:24:33Z

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.

/reviewers 2

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-8377427: Reduce substring allocations in Color.web(String, double) (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2069

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-02-08T04:25:29Z

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

openjdk · 2026-02-08T04:26:09Z

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

openjdk · 2026-02-08T04:26:35Z

@mstr2
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).

mlbridge · 2026-02-08T04:33:46Z

Webrevs

01: Full - Incremental (040dc875)
00: Full (95a305a2)

andy-goryachev-oracle · 2026-02-09T23:05:22Z

+1

andy-goryachev-oracle · 2026-02-09T23:26:13Z

modules/javafx.graphics/src/main/java/javafx/scene/paint/Color.java

+
+        if (colorString.regionMatches(true, 0, "#", 0, 1)) {
+            offset = 1;
+        } else if (colorString.regionMatches(true, 0, "0x", 0, 2)) {


interestingly, while the code (both old and new) ignores case, the javadoc says nothing about case sensitivity.

It does say that it parses a CSS color string, and CSS is case-insensitive.

andy-goryachev-oracle · 2026-02-09T23:43:50Z

modules/javafx.graphics/src/main/java/javafx/scene/paint/Color.java

+        int start = off;
+        int limit = end;
+
+        while (start < limit && Character.isWhitespace(color.charAt(start))) {


I think this change is not equivalent: the trim() function (used before) removes leading and trailing "spaces" which are defined as space is defined as any character whose codepoint is less than or equal to 'U+0020' (the space character).

Character.isWhitespace() is different:

hex| trim| isWhitespace 0x00 W - 0x01 W - 0x02 W - 0x03 W - 0x04 W - 0x05 W - 0x06 W - 0x07 W - 0x08 W - 0x09 W W 0x0a W W 0x0b W W 0x0c W W 0x0d W W 0x0e W - 0x0f W - 0x10 W - 0x11 W - 0x12 W - 0x13 W - 0x14 W - 0x15 W - 0x16 W - 0x17 W - 0x18 W - 0x19 W - 0x1a W - 0x1b W - 0x1c W W 0x1d W W 0x1e W W 0x1f W W 0x20 W W

see https://github.com/andy-goryachev-oracle/Test/blob/main/test/goryachev/research/TestWhitespace.java

While that's true, I think it is irrelevant. Color.web() is not specified to just skip over non-whitespace control characters inside of a color function, and it is not prudent to assume that a string parsing function would silently skip over non-whitespace control characters by default.

I am pointing out a non-zero chance of regression, for instance when this method is used to parse an external poorly formatted file with no sanitizing.

I do agree that this change is probably fine though.

hjohn · 2026-02-10T10:39:31Z

Color.web(string, double) parses a color string by creating substrings of the input. Almost all of these string allocations can be removed, except for an invocation of Double.parseDouble(String), which doesn't have an overload that accepts a sub-range of the input string.

Use of parseDouble is questionable anyway (unless input was sanitized first) as it is a Java double parser. For example, this works:

Color.web("rgba(2, 2, 1, 0.25e2f)")

mstr2 · 2026-02-10T15:11:52Z

Use of parseDouble is questionable anyway (unless input was sanitized first) as it is a Java double parser. For example, this works:
Color.web("rgba(2, 2, 1, 0.25e2f)")

You're right, the specification of Color.web() explicitly states: Creates an RGB color specified with an HTML or CSS attribute string.

Not every string that is parsed by Double.parseDouble() is a valid CSS number, and if it is not, it should be rejected by Color.web(). I've added code that parses a number according to CSS grammar. With this change, color parsing is now completely allocation-free.

andy-goryachev-oracle

The JavaFX CSS spec states (under ):

Real numbers and integers are specified in decimal notation only. ... A <number> can either be an <integer>, or it can be zero or more digits followed by a dot (.) followed by one or more digits. Both integers and real numbers may be preceded by a "-" or "+" to indicate the sign. -0 is equivalent to 0 and is not a negative number.

[+|-]? [[0-9]+|[0-9]*"."[0-9]+]

it looks to me like exponential notation should not be supported.

andy-goryachev-oracle · 2026-02-10T16:16:35Z

modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/parser/CssNumberParserTest.java

+        long rawBits = Double.doubleToRawLongBits(v);
+        return rawBits >= 0 ? rawBits : (0x8000_0000_0000_0000L - rawBits);
+    }
+}


missing newline

andy-goryachev-oracle · 2026-02-10T16:45:26Z

modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/parser/CssNumberParserTest.java

+        assertNumberFormatException(" 1");
+        assertNumberFormatException("1 ");
+        assertNumberFormatException("xx1e2yy", 2, 6); // slice is "1e2y"
+    }


maybe add another test for malformed numbers such as ++0, 1e--2, etc.?

andy-goryachev-oracle · 2026-02-10T16:51:15Z

modules/javafx.graphics/src/test/java/test/com/sun/javafx/css/parser/CssNumberParserTest.java

+
+    @Test
+    public void parseIntegerAndSignedInteger() {
+        assertEquals(0.0, parseDouble("0"), 0.0);


The JavaFX CSS spec also states

-0 is equivalent to 0 and is not a negative number.

How should we test this?

On the JVM, -0 == 0. It seems to me that this sentence just spells it out, doesn't it?

I don't know. The new parser parses "-0.0" as -0.0, and while (-0.0 == 0.0), the result of some other operations is different (1/-0.0 = -Infinity).

Does it constitutes a regression risk for applications?

CssNumberParser parses "-0" correctly as -0, just like Double.parseDouble() currently does. The existing code in Color is unchanged.

mstr2 · 2026-02-10T18:22:33Z

The JavaFX CSS spec states (under ):

Real numbers and integers are specified in decimal notation only. ... A <number> can either be an <integer>, or it can be zero or more digits followed by a dot (.) followed by one or more digits. Both integers and real numbers may be preceded by a "-" or "+" to indicate the sign. -0 is equivalent to 0 and is not a negative number.

[+|-]? [[0-9]+|[0-9]*"."[0-9]+]

it looks to me like exponential notation should not be supported.

In the interest of not being different just for the sake of it, we should probably change this specification and use the W3C definition of a number everywhere. Not terribly important (no one uses E notation anyway), but better be consistent.

andy-goryachev-oracle · 2026-02-10T18:56:31Z

Mandatory disclaimer: W3C is not a real standard.

I am not sure if we want to change the CSS spec to allow scientific notation in all the s, I don't even sure we want to add this support in Color.web(). If we do though, I think we would need to update the javadoc and possibly the CSS Reference, right?

andy-goryachev-oracle · 2026-02-10T19:17:11Z

I must admit I don't know how much changes in Color.web() impact CSS parsing.
For example, it looks like Color.web() is being called from CssParser.colorValueOfString(String) but a simple css like -fx-background-color: rgb(1); won't trigger that code.

andy-goryachev-oracle · 2026-02-10T19:44:54Z

modules/javafx.graphics/src/main/java/javafx/scene/paint/Color.java

-                r = Integer.parseInt(color.substring(0, 1), 16);
-                g = Integer.parseInt(color.substring(1, 2), 16);
-                b = Integer.parseInt(color.substring(2, 3), 16);
+                r = Integer.parseInt(colorString, offset, offset + 1, 16);


would a switch statement be better in this case (e.g. case when len == 1)?

mstr2 · 2026-02-10T20:12:47Z

Mandatory disclaimer: W3C is not a real standard.

5.8 billion web browser installations disagree. What fraction of a fraction of a percent of this number is JavaFX again? W3C's baseline compatibility standard is as good as any standard can possibly be. There are no other CSS specifications of any significance whatsoever.

I am not sure if we want to change the CSS spec to allow scientific notation in all the s, I don't even sure we want to add this support in Color.web(). If we do though, I think we would need to update the javadoc and possibly the CSS Reference, right?

Right now, the situation is a bit inconsistent:

Doesn't parse: rgb(1E2, 0, 0)
Parses: rgb(1E2%, 0, 0)
Doesn't parse: rgb(1E2%, 100.0, 100)
Parses: rgb(1E2%, 100, 100)

JavaFX contradicts its own specification, but the specification is (and has always been) wrong anyway.

andy-goryachev-oracle · 2026-02-10T20:57:25Z

it's not a real standard in a sense that it is an attempt to write a spec for something that every browser manufacturer want to change to make their browser "better". maybe the situation changed recently, I don't know.

But I do agree with you - the majority of developers are familiar with it, so it might make sense to adapt in some cases (while keeping in mind possible regressions).

JavaFX contradicts its own specification, but the specification is (and has always been) wrong anyway.

This is hilarious and concerning at the same time. I wonder whether we need to update the javadoc / create a CSR since we are changing the behavior. What do you think?

Reduce substring allocations in Color.web(String, double)

95a305a

openjdk bot added the rfr Ready for review label Feb 8, 2026

andy-goryachev-oracle reviewed Feb 10, 2026

View reviewed changes

CSS-compliant double parser

040dc87

andy-goryachev-oracle reviewed Feb 10, 2026

View reviewed changes

8377427: Reduce substring allocations in Color.web(String, double) #2069

Are you sure you want to change the base?

8377427: Reduce substring allocations in Color.web(String, double) #2069

Conversation

mstr2 commented Feb 8, 2026 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Uh oh!

bridgekeeper bot commented Feb 8, 2026

Uh oh!

openjdk bot commented Feb 8, 2026

Uh oh!

openjdk bot commented Feb 8, 2026

Uh oh!

mlbridge bot commented Feb 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

andy-goryachev-oracle commented Feb 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hjohn commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mstr2 commented Feb 10, 2026

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstr2 commented Feb 10, 2026

Uh oh!

andy-goryachev-oracle commented Feb 10, 2026

Uh oh!

andy-goryachev-oracle commented Feb 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstr2 commented Feb 10, 2026

Uh oh!

andy-goryachev-oracle commented Feb 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

mstr2 commented Feb 8, 2026 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 8, 2026 •

edited

Loading

hjohn commented Feb 10, 2026 •

edited

Loading