-
-
Notifications
You must be signed in to change notification settings - Fork 806
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
Content rework 2 - Electric Boogaloo #2504
Conversation
It's still used in the style chain, just not in selectors. I am unsure how to remove it since a |
Ok, I think I covered all code review items, I removed a good chunk of unsafe code and improved the existing ones in the elem macro to be a bit shorter (using the new methods on |
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.
A few more notes:
- The unpack comment was marked resolved, but not addressed.
- The Fields trait comment was marked resolved, but not addressed.
- I think the enum -> u8 should be done with as and then we can remove the
impl Into<u8>
everywhere. - I checkout the code of
smallbox
and it's full of unsafe without any safety comments :/ Are the gains really worth it?
I've pushed my changes, feel free to take a look. Main changes:
|
Looks great 🎉 |
A small update on performance improvement, we have lost a couple of percentage points due to slightly more dynamic dispatch than there was, the fact that |
Great work, thank you! 🎉 |
After the failure that was #878 I am back with version 2, it only took six months.
This also lays the groundwork for the
Value
rework discussed with @laurmaedje in the sense that it shows that structs are better than dynamic values if possible, and thatArc<dyn Trait>
is a valid and performant option.This PR is massive and has deep performance implications for typst while not changing any behaviour from the user's point of view:
struct
based instead of backed by a dynamicEcoVec
of attributes#[repr(u8)]
enum called{ElementName}Fields
(e.g forHeadingElem
it isHeadingElemFields
).255
is reserved for the"label"
"""virtual""" field (a weird workaround that predates this PR).#[internal]
are now truly internal: the user cannot access them, they no longer need to beFromValue
orIntoValue
#[internal]
changewhere()
selector now converts the field name into the ID when called and checks that the field exists on the element being selected (this is a breaking change)Arc
clones (less expensive than a full clone but involves an atomic operation and therefore cache invalidation)#[empty(<expr>)]
attribute, which allows setting a default value for a field, this is used when we need to create an empty element that is (if possible) non-allocating, this is only used when getting the supplement of figures, perhaps we can find a better way of handling this.#[not_hash]
attribute for making a field as not being part of the hashing of the element.#[borrowed]
attribute for making a field that can also be resolved from the stylechain be borrowed instead of clonedpar.rs
for linebreaking algorithm.Block
type, a type that can store a value (essentiallydyn Any
) either on the stack within a small space or on the heap using aBox<T>
. This is used in the style chain for the following change:Value
s but instead the "native" data structure used in code, to avoid doing conversion to/fromValue
everytime.Element
object-safe trait (hence why I am not re-usingNativeElement
) which allows safe access to an element in a type-erased manner.#[variant(<u8>)]
attribute to control the numerical ID of a field, this is only used forTextElem
where thetext
field is always zero andHeadingElem
.span
,location
,label
,prepared
, andguards
. These fields should probably be moved into anElementMetadata
I just haven't gotten around to that yet."mimalloc"
on the CLI, I mostly used this for testing but I cannot stress enough how huge the impact is especially on Windows. I would be tempted to have this enabled all the time but since it introduces a C dependency it's really up to @laurmaedje.ittapi
for tracing, these calls are only enabled with the feature flag"ittapi"
on the CLI and are only ever useful for profiling the code as this is the API that Intel V-Tune uses for pausing and resuming profiling, which I use specifically to profiletypst watch
. Note that I am not attached to having this included in the PR and I can easily remove it but I think it's convenient to have the calls already in the right place for those that need it (i.e just me afaik).Performance impact:
The performance impact is rather huge, not so much in cold compiles where it is about 24% on an Intel i9 12900k on Ubuntu, and 15% on an AMD 7950X3D on Windows. But in incremental, the gains are H-U-G-E, we are talking 3.5x on the 12900k and 2.5x on the 7950X3D. That makes the incremental compile times go, respectively, from 2.49s to just 0.7s and 2.25s to 0.9s. These gains are gigantic and are really noticeable on larger documents. (these tests were done with my thesis)
For @Enter-tainer's 2.5k pages document, the gains are smaller being 11% in incremental and identical in cold compile on the 7950x3d. And 16% in cold compiles and 17% in incremental compilation. Mind you, I also tested on my 7950x3D but without 3D V-cache and the gains in incremental are 80% which is amazing too.
Therefore, it is fair to say that this is dependent on the document but generally document containing lots of queries and large bibliographies should benefit the most. Since there are very few changes to the layout engine, documents that are bound by said engine (as @Enter-tainer's document is) will see very little in the way of gains. From these results as well as other benchmarks I have done in the past, it becomes clear that memory latency and cache size are major factors when it comes to the performance of Typst.
Still left to do:
ElementMetadata
instead of direct fields and repeated codeRemove usage ofCow<T>
in some places (it had no measurable performance impact)elem
macro and renameselem
(the new one) toelem