Reduce size of Expr from 80 to 64 bytes#9900
Conversation
charliermarsh
force-pushed
the
charlie/box
branch
from
e39e102 to
65e91b1
Compare
CodSpeed Performance ReportMerging #9900 will improve performances by 4.49%Comparing Summary
Benchmarks breakdown
|
|
That's a bit better... |
|
I think I can do even better though. |
charliermarsh
force-pushed
the
charlie/box
branch
2 times, most recently
from
b9f9008 to
1a64522
Compare
|
Okay, there's no longer any parser regression, and there are parser and linter improvements across the board (though many don't meet our threshold). |
charliermarsh
force-pushed
the
charlie/box
branch
from
1a64522 to
ecf5a10
Compare
charliermarsh
force-pushed
the
charlie/box
branch
from
ecf5a10 to
2543f3d
Compare
charliermarsh
force-pushed
the
charlie/box
branch
from
2543f3d to
b387317
Compare
| pub args: Vec<Expr>, | ||
| pub keywords: Vec<Keyword>, | ||
| pub args: Box<[Expr]>, | ||
| pub keywords: Box<[Keyword]>, |
There was a problem hiding this comment.
We could make these Option<Box<...>> to avoid allocating when empty...
There was a problem hiding this comment.
It should be possible to use Option without increasing the struct size, and avoiding the allocation could be nice. although It doesn't seem that the empty slice allocates
There was a problem hiding this comment.
Yeah that is interesting. I actually couldn't find how this was being done in std specifically. So it could just be an optimization in the code generator somewhere. Or perhaps I missed a call to dangling.
| pub ops: Vec<CmpOp>, | ||
| pub comparators: Vec<Expr>, | ||
| pub ops: Box<[CmpOp]>, | ||
| pub comparators: Box<[Expr]>, |
There was a problem hiding this comment.
These are never empty so zero-length slice is not a problem.
| // static_assertions::assert_eq_size!(ExprTuple, [u8; 40]); | ||
| // static_assertions::assert_eq_size!(ExprUnaryOp, [u8; 24]); | ||
| // static_assertions::assert_eq_size!(ExprYield, [u8; 16]); | ||
| // static_assertions::assert_eq_size!(ExprYieldFrom, [u8; 16]); |
There was a problem hiding this comment.
@MichaReiser - I missed your comment when we talked in-person -- did you want me to include these as assert!?
There was a problem hiding this comment.
Ah sorry. I don't mind keeping them. I do find them useful when analyzing the type's size in the future.
BurntSushi
left a comment
BurntSushi
left a comment
There was a problem hiding this comment.
Master of the double-star.
charliermarsh
force-pushed
the
charlie/box
branch
3 times, most recently
from
0863c48 to
3d93f59
Compare
charliermarsh
force-pushed
the
charlie/box
branch
from
3d93f59 to
708ef73
Compare
| pub args: Vec<Expr>, | ||
| pub keywords: Vec<Keyword>, | ||
| pub args: Box<[Expr]>, | ||
| pub keywords: Box<[Keyword]>, |
There was a problem hiding this comment.
It should be possible to use Option without increasing the struct size, and avoiding the allocation could be nice. although It doesn't seem that the empty slice allocates
|
## Summary This PR reduces the size of `Expr` from 80 to 64 bytes, by reducing the sizes of... - `ExprCall` from 72 to 56 bytes, by using boxed slices for `Arguments`. - `ExprCompare` from 64 to 48 bytes, by using boxed slices for its various vectors. In testing, the parser gets a bit faster, and the linter benchmarks improve quite a bit.
3 participants
Summary
This PR reduces the size of
Exprfrom 80 to 64 bytes, by reducing the sizes of...ExprCallfrom 72 to 56 bytes, by using boxed slices forArguments.ExprComparefrom 64 to 48 bytes, by using boxed slices for its various vectors.In testing, the parser gets a bit faster, and the linter benchmarks improve quite a bit.