Skip to content

Optimize table-like length storage#172

Merged
sasial-dev merged 5 commits intored-blox:0.6.xfrom
daimond113:feat/optimized-table-length
Apr 5, 2025
Merged

Optimize table-like length storage#172
sasial-dev merged 5 commits intored-blox:0.6.xfrom
daimond113:feat/optimized-table-length

Conversation

@daimond113
Copy link
Copy Markdown
Contributor

@daimond113 daimond113 commented Mar 12, 2025

Optimizes length storage for table-like types (map, set). It works by getting the amount of possible keys (number of variants for enums) to use for the length storage.

@daimond113 daimond113 marked this pull request as draft March 12, 2025 16:37
@daimond113 daimond113 marked this pull request as ready for review March 12, 2025 17:04
Copy link
Copy Markdown
Collaborator

@sasial-dev sasial-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the idea - but I think we need to reconsider how it's actually executed.

I'm not a big fan of resolve_ty/ variants_size personally - resolving tydecls based upon going back to the config so many times over feels stupid! I also don't want to pass the whole config to irgen.

One idea I had - maybe we should consider adding a length argument to the ty? Then we can assign it in convert.rs or whatever.
Otherwise, let me know if you have any other ideas on how we should approach this.

I know this would sorta backpedal and require a refactor of your work on #166, but I think it's for the best.

Comment on lines +328 to +344
pub fn variants_size(&self) -> Option<NumTy> {
match self {
Ty::Enum(Enum::Unit(variants)) => Some(NumTy::from_f64(0.0, (variants.len() - 1) as f64)),
Ty::Enum(Enum::Tagged { variants, .. }) => Some(NumTy::from_f64(0.0, (variants.len() - 1) as f64)),
Ty::Num(numty, ..) => match numty {
NumTy::F32 => None,
NumTy::F64 => None,
NumTy::U8 => Some(NumTy::U8),
NumTy::U16 => Some(NumTy::U16),
NumTy::U32 => None,
NumTy::I8 => Some(NumTy::U8),
NumTy::I16 => Some(NumTy::U16),
NumTy::I32 => None,
},
_ => None,
}
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is really weird.

@daimond113
Copy link
Copy Markdown
Contributor Author

I removed the number optimization since the formula for the length of a map is actually variants_size + 1 (because length 0 needs to be stored as well).

@sasial-dev
Copy link
Copy Markdown
Collaborator

I wonder if we can (or should!) make our optimisations more aggressive?

e.g. length should only be stored in a u8 for:

event Test = {
    from: Client,
    type: Reliable,
    call: SingleSync,
    data: u8[255..270]
}

@daimond113
Copy link
Copy Markdown
Contributor Author

I've been looking into making the NumTy algorithm smarter like that, but I think it's better if it becomes a separate PR.

Copy link
Copy Markdown
Collaborator

@sasial-dev sasial-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
This lays the groundwork nicely for further optimisations.

@sasial-dev sasial-dev merged commit 67940d0 into red-blox:0.6.x Apr 5, 2025
2 checks passed
@sasial-dev sasial-dev mentioned this pull request Apr 5, 2025
@sasial-dev sasial-dev self-assigned this Jun 24, 2025
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