Skip to content
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

WIP: Token map perf #1427

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

WIP: Token map perf #1427

wants to merge 1 commit into from

Conversation

jorenbroekema
Copy link
Collaborator

@jorenbroekema jorenbroekema commented Jan 14, 2025

Issue #, if available:

There are some performance related issues with having loads of references (especially chained ones) due to always having to traverse the entire tokens object to find a reference, and for each member of the chain too.

This PR is the second of a series of 3 PRs aimed at tackling this problem. The first was to create a utility to convert a tokens object to a Map structure, which makes it a lot cheaper than Array.find() or Object traversal to access the referenced tokens, while still making it convenient to iterate/loop over.

This PR completely refactors the reference resolver and token transformer lifecycle to use the Map() instead of the Object. There is still a small bit of object traversal happening for the resolver, but only at the level of the token's value property which could be a (nested) object. Besides that, it uses direct Map.get() to find a reference which is very fast.

The third PR is still to be started, which will look into caching reference lookups so that if multiple references end up resolving to the same token (e.g. because of chained refs with the last item being the same one), preventing the same work from having to be done more than once. I suspect the performance gains of this PR to be more of a bonus compared to the previous 2, and only significant for tokensets with loads of deep chained refs.

One thing to note is that this PR is a breaking change, which would mean a new major version of Style Dictionary, for 2 main reasons:

  • We're working with a flat token structure, and only Tokens are in there. Non-token nodes in the object are filtered out completely, meaning they 1) don't end up in your output anymore and 2) references cannot be made to such non-token nodes. This was behavior that was somewhat documented through tests and has been recommended as workarounds in issues, so I'm not entirely comfortable saying that "it was unintended behavior / private API, we're gonna 'fix' it in a minor bump".
  • I've removed the .value (or .$value in DTCG) suffix support in token references, this is not really aligned with the DTCG spec and imo is a feature no one really needs or should be using. It's technically possible to keep the backward compatibility for it so let's discuss whether we want to remove it fully or merely stop using it in our docs/examples.

TODOs:

  • get in some testers with real-world tokens and measure perf gaps. For some tokensets I expect a slight performance degradation due to converting from nested -> flat -> nested, the benefits are primarily for the larger sets where the benefits of resolving/transforming on a flat structure far outweigh the conversion costs.
  • work on the automated performance tests and analyze the perf graph in dev tools to confirm the performance has improved for large sets, many refs, very deep refs, and the Expand object tokens (with refs) API.
  • The resolveReferences and getReferences utils should support the Map object as input, and the Object should be deprecated for removal in the next major (targeted v6 if this PR makes for v5). I still want this v5 major to be a small drop-in replacement for 95%+ of users, breaking these utils to only allow passing in the Map would make it breaking for a significantly larger portion of our users and I want to save that for v6.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jorenbroekema jorenbroekema requested a review from a team as a code owner January 14, 2025 14:58
@jorenbroekema jorenbroekema force-pushed the token-map-perf branch 2 times, most recently from 0227587 to aa0eac2 Compare January 14, 2025 15:21
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1427.d16eby4ekpss5y.amplifyapp.com

@DarioSoller
Copy link

I have a relative small example token set of 81 tokens, that currently take about 2 minutes to build, thus could serve as a test token set for this performance PR. Please pm me for details.

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.

2 participants