Merged
Conversation
eba2b59 to
dbee301
Compare
dbee301 to
e225559
Compare
e263a81 to
b686ba5
Compare
8babe6c to
951bb31
Compare
0aee8eb to
2657e1b
Compare
2657e1b to
b990602
Compare
b112e89 to
55c0714
Compare
786b58e to
a719ab8
Compare
noahbald
commented
Nov 16, 2025
Comment on lines
+90
to
+92
| [patch.crates-io] | ||
| web_atoms = { git = "https://github.com/servo/html5ever.git", package = "web_atoms", rev = "a2ca649" } | ||
|
|
Owner
Author
There was a problem hiding this comment.
This should be remove on next xml5ever/markup5ever release
noahbald
commented
Nov 17, 2025
| else { | ||
| let computed_styles = ComputedStyles::default() | ||
| .with_all(element, &context.query_has_stylesheet_result) | ||
| .map_err(JobsError::ComputedStylesError)?; |
Owner
Author
There was a problem hiding this comment.
This is a generally benign error and doesn't need to halt jobs. We should update the handling of each job to ok and print warnings for some of the error types.
noahbald
commented
Nov 17, 2025
| @@ -446,10 +445,10 @@ fn cleanup_ids() -> anyhow::Result<()> { | |||
| <g id="g001"> | |||
| <circle id="circle001" fill="url(#gradient001)" cx="60" cy="60" r="50"/> | |||
| <rect fill="url('#gradient001')" x="0" y="0" width="500" height="100"/> | |||
| <tref href="#referencedText"/> | |||
| <tref xlink:href="#referencedText"/> | |||
Owner
Author
There was a problem hiding this comment.
note: href is unknown for tref, and so wouldn't be used as a reference. xlink:href should be used instead
noahbald
commented
Nov 18, 2025
| let values = Allocator::new_values(); | ||
| let mut arena = Allocator::new_arena(); | ||
| let mut allocator = Allocator::new(&mut arena, &values); | ||
| let dom = parse(&xml, &mut allocator).unwrap(); |
Owner
Author
There was a problem hiding this comment.
Parsing API could be simpler without so much boilerplate. Should investigate better approaches
278a614 to
9dc8cfc
Compare
9dc8cfc to
04a7c18
Compare
This was referenced Nov 19, 2025
Closed
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR extends the existing AST functionality to parse and embed known element and attribute values upfront, rather than processing them ad-hoc.
ElementIdAttrandAttrIdexpected_attribute_groupsandexpected_attributesfor eachElementIdThe AST is now provided through concrete types instead of traits. You can still bring your own parser to create a document.
Motivation and Context
Performance
The fun part: parse; dont validate. Previously, treating names and values as strings we can only derive information through expensive operations like querying data from
HashMap, regex, parsers, etc, which is wasted time.Now that we do all parsing upfront, with
Element,Attr, etc; the structure, data, and validity of these types are known ahead of time.All in all, this bring the following changes to performance (with blobs.svg)
Minification
By using parsed values, each type can implement it's own minification, further improving the efficacy of OXVG compared to similar tools.
Correctness
Now that the document fully embeds type information, the following can now be achieved.
AttrId/ElementIdwe can be confident we're handling values of the correct namespaceFuture work
Future linting and transforming work will be able to take advantage of these changes, as the expected values and capabilities of data is now embedded into the type system.
For example, the linter will be able to check which attributes were successfully parsed, rather than having to individually process all the values in the document.
For an GUI SVG editor, we'll be able to show users a set of allowed elements and attributes they can manipulate the document with.
How Has This Been Tested?
Types of changes
Fixes
Features
ElementId,AttrId, andAttrfor storing structured element dataoxvg_ast::attribute::data::*Atomtype which can store various string typesoxvg_collectionsin favour ofoxvg_ast::element::category,oxvg_ast::attribute::group, etc which are assigned to each variant.oxvg_*::errorfor each crateis_element,is_attribute,get_attributeetcElementno longer hashable, as it had a high likelihood of collisionNode,Node::Style, to hold parsed style content of a style elementBreaking changes
Due to changes in parsing
AllocatorinsteadDue to changes in Attribute and Element structure
oxvg_ast::implementationswithoxvg_ast::parse"style"feature flag. Presentation attrs moved fromstyletoattribute::data::presentation.Due to changes with computed-styles
ComputedStylesno longer part ofInfo, and should be used ad-hoc instead.ComputedStylesbehind"selectors"feature flag, as it's functionality would only be non-obviously incomplete otherwiseVisitor::prepareusesContextinstead ofContextFlagsandInfoContextFlagsqueries moved toContextVisitor::use_styleremoved in favor of querying the document directlyDue to changes with how jobs use the updated features
convert_colorsno longer supports SVGO optionsconvert_shape_to_pathnow avoids updating local_names referenced by stylesheets, to improve correctnessprefix_idsChecklist:
Blockers/Todo
oxvg_collections/src/collections.rsandoxvg_collections/src/allowed_content.rswith AST dataAttrId, even if the name is duplicated; as defaults and types may differget_attributeetc macros where possible--- config: xyChart: width: 622 height: 181 --- xychart horizontal title "Compile time (cargo clean; cargo build --profile release) (regress)" x-axis [Before, After] y-axis "Time (ms)" 0 --> 158000 bar [133000,158000]--- config: xyChart: width: 622 height: 181 --- xychart horizontal title "Default Jobs Benchmark (cargo bench) (progress)" x-axis ["archlinux-logo-dark-scalable.518881f04ca9.svg", "banner.svg", "blobs-d.svg", "Wikipedia-logo-v2.svg", "Inkscape_About_Screen_Isometric_madness_HdG4la4.svg"] y-axis "Time (µs)" 0 --> 300000 %% Before bar [3536, 673, 10160, 52090, 288660] %% After bar [830, 200, 1978, 12838, 55608]--- config: xyChart: width: 622 height: 181 --- xychart horizontal title "Parsing Benchmark (cargo bench) (regress)" x-axis ["archlinux-logo-dark-scalable.518881f04ca9.svg", "banner.svg", "blobs-d.svg", "Wikipedia-logo-v2.svg", "Inkscape_About_Screen_Isometric_madness_HdG4la4.svg"] y-axis "Time (µs)" 0 --> 25000 %% After bar [253, 80, 906, 2838, 23386] %% Before bar [61, 27, 352, 815, 5301]