Skip to content

Mark atom! allocated Atoms as having a 'static' lifetime and stop refcounting them #10948

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

lukesandberg
Copy link
Contributor

The Clone and Drop routines will treat them like inline atoms but their storage will instead be in a static item constructed by the macro

This should speed up initialization and access to atom! values since there is no more lazy lock just. Instead we just take the address of a static and tag it. This does complicate eq and hash a bit as well as as_str to handle the new location.

This follows a similar pattern used in turbopack as of vercel/next.js#81994. Things are different here due to the use of ThinArc for storage, which required me to introduce a separate struct for the &'static str case.

The motivation is simple, don't refcount values that will never be freed.

@lukesandberg lukesandberg requested a review from a team as a code owner July 26, 2025 23:07
Copy link

changeset-bot bot commented Jul 26, 2025

🦋 Changeset detected

Latest commit: f5130a9

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

codspeed-hq bot commented Jul 26, 2025

CodSpeed Performance Report

Merging #10948 will degrade performances by 11.04%

Comparing lukesandberg:static_atom (f5130a9) with main (40a1e2e)

Summary

⚡ 2 improvements
❌ 1 regressions
✅ 137 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
es/resolver_with_hygiene/typescript 774.7 ms 870.9 ms -11.04%
html/parser/parser_document/stackoverflow_com_17_05_2022 22.7 ms 22.2 ms +2.33%
html/parser/parser_document_fragment/stackoverflow_com_17_05_2022 22.6 ms 22.1 ms +2.43%

@lukesandberg
Copy link
Contributor Author

Benchmark results are interesting. Looks like it is about a regression in the hash implementation which makes sense. Will work on addressing that.

@lukesandberg
Copy link
Contributor Author

Whups, also a silly wasm32 breakage

@lukesandberg lukesandberg requested a review from a team as a code owner July 27, 2025 05:28
mertcanaltin and others added 5 commits July 26, 2025 22:31
…efcounting them

The Clone and Drop routines will treat them like `inline` atoms but their storage will instead be in a `static` item.

This should speed up initialization and access to `atom!` values since there is no more lazy lock just a simple pointer tag.  This does complicate `eq` and `hash` a bit as well as `as_str` to handle the new location

This follows a similar pattern used in turbopack as of vercel/next.js#81994.  Things are different here due to the use of `ThinArc` for storage, which required me to introduce a separate struct for the `&'static str` case.
No public APIs have been modified so a patch release is approrpriate
@lukesandberg
Copy link
Contributor Author

well, it looks like the as_str and eq modifications are quite harmful.

The issue with Atom compared to RcStr is about the ThinArc representation. My attempts to adopt this in turbopack were not fruitful. So im guessing the key issue is that turbopack spends a lot of time cloning and dropping, so optimizing that is worthwhile. On the other hand many of the RcStr values we construct are 'de-novo' so being able to move a String is important. Perhaps in swc many of the atoms are copied out of an existing buffer, so the copy into a ThinArc is required.

Reverting to a draft while i consider this

@lukesandberg lukesandberg marked this pull request as draft July 27, 2025 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants