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

Use smol_str for identifiers #565

Closed
Razican opened this issue Jul 15, 2020 · 6 comments
Closed

Use smol_str for identifiers #565

Razican opened this issue Jul 15, 2020 · 6 comments
Labels
enhancement New feature or request memory PRs and Issues related to the memory management or memory footprint. performance Performance related changes and issues

Comments

@Razican
Copy link
Member

Razican commented Jul 15, 2020

Currently, we use a partially optimised version of String for identifiers and function names parsing and lexing. While it's OK, we could greatly benefit from using small strings that will usually be stored inline.

The smol_str crate was made for this, and it's being used on rust-analyzer, for example. It will store up to 22-bytes (ASCII characters) inline, in the stack, and then do some optimisations for whitespace strings. It will then store bigger strings in Rc'd heap allocations. It uses immutable strings, though, but identifiers don't change.

A nice review on this and other crates was done by Amos here.

This should especially help with minified code, that we will be benchmarking in #564.

This is waiting for #564 and #559.

@Razican Razican added enhancement New feature or request performance Performance related changes and issues memory PRs and Issues related to the memory management or memory footprint. labels Jul 15, 2020
@Razican Razican added this to the v0.10.0 milestone Jul 15, 2020
@Razican Razican added the blocked Waiting for another code change label Jul 15, 2020
@neeldug
Copy link
Contributor

neeldug commented Jul 15, 2020

After looking into lasso's usage, specifically for node identifiers, at which scope would you want the cache to be kept at?

@Razican
Copy link
Member Author

Razican commented Jul 15, 2020

After looking into lasso's usage, specifically for node identifiers, at which scope would you want the cache to be kept at?

lasso would be an interner (related to #279). In any case, I think the lexer should have the interner, and then, once the parser finishes parsing, it shoulg return that interner for the interpreter to use.

But in fact, this might be easier if this was a property of a Boa structure, that would have the lexer, the parser and the interpreter too.

@neeldug
Copy link
Contributor

neeldug commented Jul 15, 2020

After looking into lasso's usage, specifically for node identifiers, at which scope would you want the cache to be kept at?

lasso would be an interner (related to #279). In any case, I think the lexer should have the interner, and then, once the parser finishes parsing, it shoulg return that interner for the interpreter to use.

But in fact, this might be easier if this was a property of a Boa structure, that would have the lexer, the parser and the interpreter too.

Was looking into the property of a Boa structure, in doing so would the constructor have to call the constructor of the lexer, the parser and the interpreter, or would those be passed into the constructor to hold as a pointer. I also was trying to make the structure for the interner less convoluted by generating its own method, but can't access the Key type as it's private, if you have any suggestions to get around this, it would be much appreciated.

@neeldug
Copy link
Contributor

neeldug commented Aug 27, 2020

@Razican this can be unblocked now right? As both changes have now landed.

@HalidOdat HalidOdat removed the blocked Waiting for another code change label Aug 27, 2020
@neeldug
Copy link
Contributor

neeldug commented Sep 8, 2020

Also @Razican currently the type being used is RcString for tokens correct?

@jasonwilliams jasonwilliams modified the milestones: v0.10.0, v0.11.0 Sep 29, 2020
@Razican Razican modified the milestones: v0.11.0, v0.12.0 Jan 11, 2021
@Razican Razican modified the milestones: v0.12.0, v0.13.0 May 22, 2021
@raskad raskad modified the milestones: v0.13.0, v0.14.0 Sep 25, 2021
@Razican
Copy link
Member Author

Razican commented Jan 31, 2022

This will no longer be implemented, as interned strings will be used instead.

@Razican Razican closed this as completed Jan 31, 2022
@Razican Razican removed this from the v0.14.0 milestone Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request memory PRs and Issues related to the memory management or memory footprint. performance Performance related changes and issues
Projects
None yet
Development

No branches or pull requests

5 participants