perf(es/ast): Box Stmt and ModuleDecl
#7034
Conversation
![kodiakhq[bot]](https://avatars.githubusercontent.com/in/29196?s=60&v=4){kind=small}
| @@ -76,7 +76,7 @@ impl Take for Module { | |||
| pub struct Script { | |||
| pub span: Span, | |||
|
|
|||
| pub body: Vec<Stmt>, | |||
| pub body: Vec<Box<Stmt>>, | |||
There was a problem hiding this comment.
I think this shouldn’t be boxed become it’s already in a vec? There are some other places this is done too that could be reverted.
There was a problem hiding this comment.
Sorry. I was not thinking and it's late here :)
| #[tag("*")] | ||
| Stmt(Stmt), | ||
| Stmt(Box<Stmt>), |
There was a problem hiding this comment.
Why not just box the large variants? I'm doubtful this is going to have much impact on performance. Have you been able to benchmark it? Is this the main change in this pr?
There was a problem hiding this comment.
Yeah I profiled it using the minifier
There was a problem hiding this comment.
The performance gain was about 10% (8% ~ 15%), on total time
There was a problem hiding this comment.
Hmm... I can't reproduce it.
There was a problem hiding this comment.
If there are gains, I bet most of it is from the change on BlockStmt because that will be more prevalent than top level module items (generally, most files will have a low number of module items). It seems most of the nightly feature is used to support the boxing here though.
There was a problem hiding this comment.
Sounds reasonable. I'll wait for the profiling result as I requested one to someone
|
I profiled this again, and seems like this also regresses performance |
|
I don't think boxing outside is going to work, because you're essential allocating twice for each stmt - elements of Shrinking the struct only takes effect in places where we clone the whole |
|
There are lots of codes that replace |
|
If you clone it, this patch does not help. It's a new allocation anyway. Tihs PR is about removing needless memory operations, not required operations. |
|
Closing in favor of #7041 |
|
Well, I think I may need this/ |
There are many prepend_stmt operations. Should we allocate a separate field for the directive "use strict"? Alternatively, is there any other data structure that would facilitate our insertion operations? |
|
|
|
I'll create another PR in the future |
Description:
Related issue: