RFR: 8377427: Reduce substring allocations in Color.web(String, double) [v7]
Andy Goryachev
angorya at openjdk.org
Wed Feb 18 18:48:29 UTC 2026
On Wed, 18 Feb 2026 08:28:56 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:
>> OK, I am not against doing this in principle. But it's a new, non-trivial algorithm that needs to have additional tests and the implementation needs to be reviewed. The PR description should also change to reflect this added complexity.
>
>> OK, I am not against doing this in principle. But it's a new, non-trivial algorithm that needs to have additional tests and the implementation needs to be reviewed. The PR description should also change to reflect this added complexity.
>
> The tests are there to proof that it parses doubles correctly. The algorithm can be treated as a black box. It doesn't need to be exhaustively checked, that has already been done far more thoroughly than we can hope to do -- it's like having to prove that some complex sorting algorithm works; you don't need to fully understand the algorithm, just verify some edge cases, to proof you didn't make a mistake "copying" the proven algorithm.
>
> I checked what Michael copied, and I didn't see any mistakes. The test cases (and probably numerous dependent tests in the rest of FX) proof that it works.
Let me explain. One of the reasons we do code review is to understand the change sufficiently enough in order to be able to maintain it, possibly over decades. the code review process requires non-zero effort, so please be patient with us (and thanks for reviewing it!)
Many thanks to @mstr2 for including the reference to the original papers, but still - it is a novel, non-trivial implementation, which raises the bar significantly.
I've asked a few questions, it would be nice to see them addressed at some point:
- some measurements of the improvement in parsing performance
- more tests to demonstrate the correct behavior, esp. near transition points
- updated PR description (and possibly JBS as well) mentioning the new algorithm
- document the changes in behavior (a whitespace handling is one, are there other changes?)
- do the behavior differences cross the threshold for this to require a CSR?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2069#discussion_r2823888953
More information about the openjfx-dev
mailing list