-
-
Notifications
You must be signed in to change notification settings - Fork 502
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
perf(oxc_ast): reduce ast memory usage #5645
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
3845b15
to
83ae461
Compare
CodSpeed Performance ReportMerging #5645 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid these changes so far don't make sense to me. The indirection of Box
has a cost, so generally speaking should be avoided unless necessary.
In my opinion, the fields which we should consider boxing are either:
- Large and wrapped in a
Option
, which is oftenNone
. Vec
s which are commonly empty.
e.g.:
Class::decorators
(most classes aren't decorated, so thisVec
is usually empty).FormalParameter::decorators
(ditto)FunctionBody::directives
(most functions don't have any directives)ArrayAssignmentTarget::rest
([a, b] = arr
is more common than[a, ...b] = arr
, sorest
is usuallyNone
)
In particular, I imagine that the majority of code that Oxc will run against will be plain JS (when bundling an application, the volume of code in node_modules
will likely dwarf the application code written in TS). Therefore I think we should optimize the AST to be as memory-efficient as possible for JS, by boxing large TS-only fields.
Side note: We can expect Vec
to be reduced to 16 bytes in future, which will reduce the size of a lot of types. oxc-project/backlog#18
@@ -597,7 +597,7 @@ pub struct PrivateFieldExpression<'a> { | |||
pub struct CallExpression<'a> { | |||
#[serde(flatten)] | |||
pub span: Span, | |||
pub callee: Expression<'a>, | |||
pub callee: Box<'a, Expression<'a>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't make sense to me. It introduces indirection to save 8 bytes. And it's not really saving any bytes anyway as the Expression
is still stored in arena, just in a separate allocation - in fact overall it costs an extra 8 bytes for the additional pointer.
part of #5601