-
Notifications
You must be signed in to change notification settings - Fork 109
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
Add CSS calc #668
Add CSS calc #668
Conversation
Dimension, LengthPercentage and LengthPercentageAuto
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 looks broadly good to me. I wouldn't bother with a no_std implementation (alloc should be trivial I think). What I would ideally like is for this to be feature-flagged behind a calc
feature. Possibly the best way to deal with feature flagging all the .clone()
s is to define a new method on Dimension
, etc which calls .clone()
if the feature flag is enabled and consumes and returns self
otherwise. That way the conditional compilation is largely restricted to one place.
However we have a bit of a problem, which is that I am seeing benchmark regressions (run cargo xbench
from the root or cargo bench
from the benchmarks
directory) averaging around +50%. Which is rather a large overhead! And this is on benchmarks without any actual Calc
values too. There are two obvious culprits for this:
- The increase in size of the style types
- Calls to
.clone()
not being able to be as easily optimised as memcopy's.
As a quick test I did a find/replace on Taffy's main
branch, replacing all f32
's with f64
s, and this seemed to result in roughly a 20% performance hit. This suggests to me that it might be a bit of both.
My suggestions for resolving this would be either:
- The combination of:
a. Using a "tagged pointer" representation (aunion
) to squash aDimension
down to 64 bits
b. Adding aDimensionRef
type or similar that borrowsDimension
to cut down on clones where possible. Methods likeresolve_or_zero
also ought to possible to make to work without cloning.
c. As Taffy never holds persistent copies of style types, we could also consider playing fast-and-loose with the borrow checking rules and storing a raw pointer (which could beCopy
!) - Creating a new global (per-
TaffyTree
instance) Vec/Slotmap ofCalc
s. And expecting users of Taffy to register theirCalc
values there before using the index/id inDimension
(which could potentially be made 32 bits if we did this). Resolving ofCalc
s would then be done via theLayoutPartialTree
trait or similar.
Small perf improvements Refining some of the api
(This is getting forced push a few times)
@kokoISnoTarget I've been reading some of your commits, and regarding the "union", I suspect you may actually want a newtype around a |
@nicoburns I'm not quite sure what the problem of my current approach is, but I appreciate the feedback. |
3390f81
to
53a6378
Compare
Merge main into calc
Objective
Fixes: #225
Context
#232 (comment)
https://drafts.csswg.org/css-values/#funcdef-calc
Feedback wanted
Todo