Skip to content

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.

4 participants