Skip to content

[TASK] Always calculate selector specificity on demand #1028

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 3, 2025

Conversation

oliverklee
Copy link
Collaborator

@oliverklee oliverklee commented Feb 27, 2025

This avoids additional state that makes it hard to compare
selectors for equality.

Also, this will improve performance for cases where the
specificity is not needed (but will slightly worsen
performance where the specificity is queried multiple times
on the same selector instance).

Closes #1021

@oliverklee oliverklee self-assigned this Feb 27, 2025
@oliverklee oliverklee marked this pull request as draft February 27, 2025 19:09
@coveralls
Copy link

coveralls commented Feb 27, 2025

Coverage Status

coverage: 55.829% (-0.2%) from 56.006%
when pulling 8570e6b on task/specificity-on-demand
into 54ca442 on main.

@oliverklee oliverklee force-pushed the task/specificity-on-demand branch 6 times, most recently from 2d60478 to 412a2df Compare March 1, 2025 09:45
@oliverklee oliverklee changed the title [TASK] Stop caching the selector specificity [TASK] Always calculate selector specificity on demand Mar 1, 2025
@oliverklee oliverklee force-pushed the task/specificity-on-demand branch 2 times, most recently from 26eab63 to 48d8842 Compare March 1, 2025 11:02
@oliverklee oliverklee marked this pull request as ready for review March 1, 2025 11:04
@oliverklee oliverklee requested a review from JakeQZ March 1, 2025 11:04
Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep the optimization, as it would give a significant performance improvement, when, for example, sorting selectors by specificity, which would be quite a common use case. As long as the cached value if cleared when setSelector is called (which it is), I don't see it causing a problem.

We should, however, still remove the setting of the cached value in the constructor, as this is unnecessary (and a performance hit).

@oliverklee
Copy link
Collaborator Author

Calculating the value lazily and storing the cached value in Selector makes testing with this class quite hard (as can be seen in #1021).

To help solve this problem and still keep performance high, I've now created a caching utility class for this in #1049.

@oliverklee oliverklee marked this pull request as draft March 2, 2025 07:44
@oliverklee oliverklee force-pushed the task/specificity-on-demand branch from 48d8842 to b7d1926 Compare March 2, 2025 14:08
This avoids additional state that makes it hard to compare
selectors for equality.

Also, this will improve performance for cases where the
specificity is not needed (but will slightly worsen
performance where the specificity is queried multiple times
on the same selector instance).

Closes #1021
@oliverklee oliverklee force-pushed the task/specificity-on-demand branch from b7d1926 to 8570e6b Compare March 2, 2025 19:36
@oliverklee oliverklee marked this pull request as ready for review March 2, 2025 19:38
@oliverklee
Copy link
Collaborator Author

I have now changed `Selector' to use the new utility class.

@oliverklee oliverklee requested a review from JakeQZ March 2, 2025 19:39
Copy link
Collaborator

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat.

@JakeQZ JakeQZ merged commit 86aeaa7 into main Mar 3, 2025
21 checks passed
@JakeQZ JakeQZ deleted the task/specificity-on-demand branch March 3, 2025 01:25
@JakeQZ
Copy link
Collaborator

JakeQZ commented Mar 3, 2025

I removed the bit in the commit message about performance downgrade, since this is no longer the case.

@oliverklee
Copy link
Collaborator Author

I removed the bit in the commit message about performance downgrade, since this is no longer the case.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants