RFR: JDK-8322964 Optimize performance of CSS selector matching [v2]
Kevin Rushforth
kcr at openjdk.org
Thu Jan 11 23:10:30 UTC 2024
On Thu, 11 Jan 2024 22:59:33 GMT, Andy Goryachev <angorya at openjdk.org> wrote:
>> I can undo the rename so they're not touched. My problem with fixing this is that I then also should do it for `CompoundSelector`, which is just completely unrelated to the intent of this PR. I'm pretty sure that within JavaFX we don't change lines that are unrelated to the intent of the change (these lines were only hit because of a clarifying field rename related to this change).
>>
>> Normally I would do such fixes in an extra commit, where the first commit would be something like "fix: Improve selector match performance" and the second is "fix: Fix bug in Selectors hashCode algorithm"...
>>
>> ... but since everything is squashed as the way of working in FX (instead of having stand alone commits that are fast forwarded when included in master), it would have to be done the official way, which is file a ticket, create a new PR, have it also reviewed, etc. This puts an additional burden on the already long review queue, with well intentioned solid PR's that are abandoned because of lack of feedback and reviews... Even simple things like fixing warnings I've given up on as I see it taking time away from more pressing issues.
>>
>> So, I can file a ticket and create another PR.
>
> I feel the same kind of frustration sometimes. The review process is indeed painfully slow.
>
> A different JBS/PR would be fine, I think, something along the lines of "suboptimal hashCode() implementation in ...". I also think it's ok to keep the new names (so you don't have to do extra work) since there will be a new PR.
I agree with John that changing the existing hash algorithm is really out of scope for this PR (even if it is suboptimal, which it is).
Filing a separate bug has the advantage of decoupling it in time as well as not burdening this review. It may or may not even be worth implementing the optimization if no one uses it, but in any case, it can be done later.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1449506858
More information about the openjfx-dev
mailing list