Skip to content

🏎️ feat: expose calculateForAST method #23

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 6 commits into from
Feb 11, 2025

Conversation

bartveneman
Copy link
Contributor

@bartveneman bartveneman commented Feb 18, 2024

closes #17

  • Exposes calculateSelectorNode next to the existing calculate function
  • Adds static Specificity.calculateSelectorNode(selector: string) + tests
  • Moved around some internals so calculateSelectorNode does not need to reference Specificity each time
  • Agree on naming
  • Check that /test/standalone is implemented correcty. I suspect the existing tests for calculate don't actually test it's implementation so I did mine a little different than the existing tests. Please double-check my work 😅. Also opened Question: are standalone tests correct? #24 for further discussion.

@bramus
Copy link
Owner

bramus commented Jun 20, 2024

Apologies for the delay but only now I took a good look into this PR.

One thing I’m wondering is why calculateSelectorNode returns a plain object while calculate does return (an array) with Specificity objects. I would have assumed for both to return Specificity instances

@bartveneman
Copy link
Contributor Author

bartveneman commented Jun 20, 2024

~~The existing function already returned a single object, so I simply renamed it and exported it.

It does make sense to me however to keep this method as small as possible, because it's easy to integrate and mock for testing.~~

edit: interpreted your question incorrectly

For my use case I generate sometimes over 10,000 Specificity instances, each with a small enough footprint. Separating instantiating the Specificity object felt light the right thing to do, to keep more of the 'business logic' in the more customer-facing calculate method. On the other hand, I don't have very strong feelings about this, so happy to revert that part.

@bartveneman
Copy link
Contributor Author

bartveneman commented Aug 10, 2024

My css-analyzer package has benchmarks enabled in CI and running it with the changes from this MR makes the analyzer 25-50% faster! projectwallace/css-analyzer#423 (comment)

(I don't necessarily trust the benchmarks blindly and ~50% is probably waaaay too much, but I'm very confident that it's faster because it's making a lot fewer function calls and memory allocations)

@bramus
Copy link
Owner

bramus commented Feb 10, 2025

I did some local performance testing:

  • Specificity.calculate(string) which passes a string into Specificity.calculate()
  • Specificity.calculate(ast) which passes a pre-parsed AST into Specificity.calculate()
  • Specificity.calculateSpecificityOfSelectorObject(ast) which passes a pre-parsed AST into Specificity.calculateSpecificityOfSelectorObject()

(This uses an unmodified package, all invocations return one Specificity instance – possibly wrapped in an array)

These are the results:

Specificity.calculate(string) x 404,178 ops/sec ±1.19% (96 runs sampled)
Specificity.calculate(ast) x 7,517,796 ops/sec ±1.52% (96 runs sampled)
Specificity.calculateSpecificityOfSelectorObject(ast) x 48,482,376 ops/sec ±0.94% (96 runs sampled)

What is interesting is that the simple check if (ast.type === 'Selector') in Specificity.calculate() is enough to slow things down by 85% (!). So yeah, exposing calculateSpecificityOfSelectorObject would definitely allow speeding things up.

As for the name, what about calculateForAST or maybe calculateForSelectorAST? I believe having the name AST in there is pretty clear to indicate what it’s about.

As for changing the return format (see https://github.com/bramus/specificity/pull/23/files#diff-b4fecae0a093d01e2d48a4410d338901e0e6bf09a6ef3b2a91c4dcf34b2cd61dR184), I would not do that as the package is meant to return Specificity instances.

I see no significant impact when I re-run all benchmarks when the functions return a simple object instead of a Specificity instance:

Specificity.calculate(string) x 411,932 ops/sec ±0.67% (91 runs sampled)
Specificity.calculate(ast) x 7,899,297 ops/sec ±0.28% (98 runs sampled)
Specificity.calculateSpecificityOfSelectorObject(ast) x 49,126,613 ops/sec ±1.63% (95 runs sampled)

(Yes there is a difference, but I don’t see a huge impact)

@bartveneman
Copy link
Contributor Author

Excellent, excellent research! Shall I update the PR with these proposed changes? It'll solve my use case so I'm in favour and I'm happy to spend some more time on this if you want. 🙌

@bramus
Copy link
Owner

bramus commented Feb 10, 2025

Please do. Afterwards I can commit the benchmarks too and then prepare a new release :)

(And apologies for the delay, should have looked into this much much sooner)

@bartveneman bartveneman changed the title expose calculateSelectorNode 💪 feat: expose calculateForAST method Feb 11, 2025
@bartveneman
Copy link
Contributor Author

  • Updated method name and signature
  • Added static calculateForAST() to .d.ts because I forgot about that in the first round

@bartveneman bartveneman changed the title 💪 feat: expose calculateForAST method 🏎️ feat: expose calculateForAST method Feb 11, 2025
@bartveneman bartveneman requested a review from bramus February 11, 2025 15:25
@bramus bramus merged commit 9953e0f into bramus:main Feb 11, 2025
3 checks passed
@bartveneman bartveneman deleted the expose-calculate-obj branch February 11, 2025 15:28
@bartveneman
Copy link
Contributor Author

Thanks for getting this across the finish line!

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.

Consider exposing calculateSpecificityOfSelectorObject
2 participants