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

Reduce size of ReferenceId and SymbolId in AST #1

Closed
overlookmotel opened this issue May 15, 2024 · 5 comments
Closed

Reduce size of ReferenceId and SymbolId in AST #1

overlookmotel opened this issue May 15, 2024 · 5 comments
Assignees

Comments

@overlookmotel
Copy link

ReferenceId and SymbolId are u32. In the AST, they're stored as Cell<Option<Id>>.

https://github.com/oxc-project/oxc/blob/b9d69ad665ab386233f940e9f1a9b3095a2238dc/crates/oxc_ast/src/ast/js.rs#L428
https://github.com/oxc-project/oxc/blob/b9d69ad665ab386233f940e9f1a9b3095a2238dc/crates/oxc_ast/src/ast/js.rs#L456

semantic's ScopeTree also contains parent_ids: IndexVec<ScopeId, Option<ScopeId>>

https://github.com/oxc-project/oxc/blob/b9d69ad665ab386233f940e9f1a9b3095a2238dc/crates/oxc_semantic/src/scope.rs#L23

u32 has no niche, so Option<u32> is 8 bytes.

This makes oxc::ast::IdentifierReference 8 bytes larger than it needs to be, because its reference_flag: ReferenceFlag field is an extra byte, but gets followed by 7 bytes of padding. IdentifierReference is very common in the AST.

We should either:

  1. Make these IDs NonZeroU32 to give them a niche.
    or
  2. Don't use Cell<Option<Id>>. Just use Cell<Id> and use 0 as a default value. 0 can mean "global scope" or "undefined".

I prefer option 2. Then we don't have to unwrap the Option all the time, which may remove panic code.

@Boshen Boshen self-assigned this May 15, 2024
@Boshen
Copy link
Member

Boshen commented May 16, 2024

Can I use https://crates.io/crates/nonmax

This didn't exist 6 months so I was too lazy to add a NonZeroU32 due to its awkward API.

rustc has an internal NonMax* type.

@Boshen
Copy link
Member

Boshen commented May 16, 2024

I hate rust

define_index_type! {
    pub struct ReferenceId = NonMaxU32;
}
error[E0599]: no function or associated item named `max_value` found for struct `NonMaxU32` in the current scope
   --> crates/oxc_syntax/src/reference.rs:7:1
    |
7   | / define_index_type! {
8   | |     pub struct ReferenceId = NonMaxU32;
9   | | }
    | |_^ function or associated item not found in `NonMaxU32`
    |
note: if you're trying to build a new `NonMaxU32` consider using one of the following associated functions:
      NonMaxU32::new
      NonMaxU32::new_unchecked
   --> /Users/boshen/.cargo/registry/src/index.crates.io-6f17d22bba15001f/nonmax-0.5.5/src/lib.rs:395:1
    |
395 | nonmax!(unsigned, NonMaxU32, NonZeroU32, u32);
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    = note: this error originates in the macro `define_index_type` which comes from the expansion of the macro `nonmax` (in Nightly builds, run with -Z macro-backtrace for more info)

Same error for NonZeroU32.

@Boshen
Copy link
Member

Boshen commented May 16, 2024

Ahhh

https://github.com/thomcc/index_vec/tree/master?tab=readme-ov-file#what-features-are-planned

Allow index types such as NonZeroU32 and such, if it can be done sanely.

@Boshen
Copy link
Member

Boshen commented May 16, 2024

Good news is we forked index_vec :-)

@Boshen
Copy link
Member

Boshen commented May 16, 2024

Open to public oxc-project/oxc#3318

@Boshen Boshen closed this as completed May 16, 2024
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

No branches or pull requests

2 participants