-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Use better string type #4946
Comments
@kdy1 While working on #2175, I've noticed that serializing strings ( Can I ask a few questions:
Please let me know if you have time. I may be able to make a PR. |
|
Thanks for swift reply. Sorry for more questions, but:
swc/crates/swc_atoms/src/lib.rs Line 151 in aef6de4
swc/crates/swc_atoms/src/lib.rs Lines 159 to 160 in aef6de4
Sorry for all the questions. I'd like to make a PR for this, but I don't fully understand your intention. |
It's useful for some common strings (like
We can make
Using interned string everywhere, cross-module/cross-thread seems like a wrong design to me.
Nope.
Nope. It should never share the atom generator. Otherwise the situation will become more worse.
I already patched required things.
It will be useful but I'm not sure about the way to do it without another pointer indirection. |
Thanks for answers. OK, I think I get it - no sharing between threads. What did you have in mind for passing the Would the
|
We don't need to share But ti's not true tor other places |
RecapTo recap, my understanding is: Old
Both the "static" and "dynamic" stores are global and shared across all threads. Advantages of
Disadvantages of
New
Possible combined approachI wonder if we can combine the advantages of both into a single entity, which can be used everywhere.
In some places, the string is very unlikely to be repeated e.g. comments, so a second entity ImplementationProbably simplest way would be to fork and amend string_cache and phf to:
rust-phf/rust-phf#88 (comment) suggests It'd be ideal to get these changes implemented upstream, but maybe better get it working in SWC first and then see if can get those changes merged upstream after. rust-phf/rust-phf#236 has been open for over a year. QuestionI'm confused about one thing: PR #5271 switched over to My understanding is that So I'm confused - in what circumstances are Sorry this is long. I'm coming into this cold, and I have probably missed a lot of previous conversations. |
We don't use lock while creating it but we pass it to other threads. |
Thanks for fast reply. Why/where is Would be very interested in your thoughts on the rest of this idea when you have time. |
I was actually planning something similar, phf + per-thread interned string without a mutex. swc/crates/swc_ecma_minifier/src/compress/pure/mod.rs Lines 173 to 193 in a37fdca
For example minifier has lots of stateless logic |
Ah. Interesting. Another question: Outside of the parser, where are most new
swc/crates/swc_ecma_transforms_base/src/hygiene/mod.rs Lines 59 to 67 in 9eb0409
Is there anywhere else which creates a lot of new What I'm thinking is, if it's rare for new
|
A small prototype here: https://github.com/overlookmotel/swc/blob/new-jsword/crates/swc_atoms/src/new_jsword.rs Code is probably full of errors, and I have no idea if this will work anyway, but it was fun to write! |
We need atomic operation even if it's read-only. |
Oh arena? Yeah it's possible but it's too much restriction for rust-side users. |
It was the dynamic set I was saying I think could avoid I need to read more about concurrency in Rust. I'll come back to you. In the meantime, if you'd have time to answer my other question, that'd be really useful:
|
Strings are mostly not static, because we don't add atoms for injected helpers or variables |
I think we should implement |
We would like to know if there are some works that the community could contribute. |
@iheyunfei It would be really nice if someone adds support for static atoms to |
I wonder, if servo/string-cache#268 is merged, whether we should still investigate a better string type for swc? |
I don't think we need a new type, then. |
@hyf0 @kdy1 FYI that PR was merged, but there were some issues (prior breaking change got published together, and then it turned out the PR's change caused a debug assert). I my fix for the debug assert was merged, we're just waiting on a new version of string-cache getting published now. |
@phoenix-ru You can create |
@kdy1 After updating to upd. Clarification: I removed all usages of a upd2. A single-threaded static set sounds like it can benefit both single-threaded and multi-threaded usage. |
@phoenix-ru Can you try newer version? |
@kdy1 This is terribly bad. |
I reverted to Summarizing:
|
I think you are doing something wrong, see vercel/turborepo#6343 (comment) |
Can you share some code? |
Ah. You must use https://rustdoc.swc.rs/swc_atoms/struct.AtomStore.html |
@kdy1 From here you can pick whatever part you want to see: parsing/transforming/codegen: https://github.com/phoenix-ru/fervid/blob/592025308719345388d6b3775276dc134e9458d9/crates/fervid/src/lib.rs#L57 (use What should I do to use AtomStore? I did an Atom polyfill like that for a moment: https://github.com/phoenix-ru/fervid/blob/592025308719345388d6b3775276dc134e9458d9/crates/fervid_core/src/structs.rs#L7 |
You should patch your parser to have a field typed |
You can call like |
Is this the final design? Can |
Yes, it's final design. Actually the problem of |
It should never be static. That's the whole point of transition from |
@phoenix-ru I managed to make it fast by default. dudykr/ddbase#14 |
@kdy1 Thanks! Even though you said
With pub static mut ATOM_STORE: Lazy<AtomStore> = Lazy::new(|| AtomStore::default());
#[macro_export]
macro_rules! fervid_atom {
($lit: literal) => {{
let store = unsafe { &mut fervid_core::ATOM_STORE };
let atom = store.atom($lit);
atom
}}
}
Do you mean I can use |
Yeah. It will use per-thread |
Yeah, I saw the sources and was confused before I noticed FYI, benched the I also compared |
This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you. |
Using
JsWord
everywhere was my mistake. We should useArc<str>
instead ofJsWord
for some types.But I'll use a wrapper named
Atom
Tasks
Add
Atom
andAtomGenerator
. (perf(common): AddAtom
type #4945)Improve
Atom
(perf(atoms): Add inlining support toAtom
#5326)Atoms should be (conceptually)
but with pointer tagging.
Add thread-local interner
Use it in es ast
Use it in css ast
Use it in html ast
The text was updated successfully, but these errors were encountered: