On Wed, 18 Feb 2026 20:33:44 GMT, Michael Strauß <[email protected]> wrote:
>> 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?
>
>> some measurements of the improvement in parsing performance
>
> I've run a JMH benchmark where I call `Color.web("rgb(x%, y%, z%)")` with
> random values. The strings are precomputed to make sure that I don't measure
> the string concatenation. Here are the results:
>
> Benchmark Mode Cnt Score Error Units
> ColorParsingPerfTest.parseColor(old) thrpt 5 619,957 ± 5,670 ops/s
> ColorParsingPerfTest.parseColor(new) thrpt 5 1545,581 ± 13,451 ops/s
>
>
>> more tests to demonstrate the correct behavior, esp. near transition points
>
> Good point, I've added more tests especially around subnormals and limits.
>
>> updated PR description (and possibly JBS as well) mentioning the new
>> algorithm
>
> I've mentioned the algorithm in the PR description.
>
>> 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?
>
> `Color.web()` parses CSS numbers, so the current behavior is out of spec.
> This just brings it in line with the specification, which doesn't require a
> CSR.
it looks like there is about 2x performance improvement for the method. this
is good, the question is - how many times the method is called in a typical
application, and what is the actual impact of it?
granted, there are many more moving parts and uncertainties that depend on the
concrete application, my question is whether the actual improvement is 0.1%, or
1%, or 10% ?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/2069#discussion_r2829105823
