RFR: JDK-8322964 Optimize performance of CSS selector matching [v2]
Andy Goryachev
angorya at openjdk.org
Thu Jan 11 23:03:32 UTC 2024
On Thu, 11 Jan 2024 22:50:32 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:
>> you are right, this code does not affect performance (I could not hit a break point here either).
>> still, since you are touching these lines, why not do it right?
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1316#discussion_r1449495652
More information about the openjfx-dev
mailing list