-
Notifications
You must be signed in to change notification settings - Fork 44
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
Rewrite rbx_xml to use quick_xml instead of xml-rs #280
Closed
Conversation
This file contains 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
Simplify logic of XmlReader::eat_text
- BinaryString no longer wrapped in cdata - special floats always serialize in uppercase
BrickColor, Ref, SharedString
Serialize -0 as "0" instead of "-0"
Add deserializer tests for PhysicalProperties
Maintains compatibility across new and old version
Rather than work out what work I'm missing since I started this rewrite, I'm going to close this and instead focus on a less invasive rewrite. I think it will result in a much faster turn around for the port, and in turn it should end up being easier to review. Sad to see my work go, but it will not die here. |
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.
rbx-xml
is slow and has trouble processing unicode, among other issues. This can and is solved by simply swapping to a new XML parser. Given the speed, I've decided to go withquick-xml
.Unfortunately, the current parser (
xml-rs
) is intertwined rather thoroughly withrbx-xml
's implementation and swapping it out forquick-xml
would be impossible without refactoring large segments of it. Given there's a few other design choices that I wanted to change, I decided to bite the bullet and go for a full rewrite. Initial testing is good and suggests we're looking at 15x the parsing speed in some cases. I have tried to prioritize not copying data when possible, so overall memory should also be low.Update: This is mostly done now, I believe. Many many tests need to be written but the bulk of the code will not change. With that in mind, here are my thoughts beyond "reviewing this is a nightmare":
This rewrite fundamentally changes how the crate is laid out. Specifically, it's split
serializer
anddeserializer
into dedicated modules and the code for (de)serializing data types now lives under those modules. This differs dramatically from the current system we use that implementsXmlType
for each type and both deserialization and serialization in one place. In my opinion, this is easier to reason with but it is significantly different and may require us to point contributors in the right direction if they're used to the old system. Also, it means we have the potential for element name mismatches between serializing and deserializing. Testing will absolutely catch that, but it's something we didn't have to deal with before.The deserializer now requires the
referent
attribute to be set onItem
elements. I was very surprised to learn it was not before, but as a result this is also a breaking change. I don't think we should care though.For posterity, here's a checklist of what needs to be done:
ReflectionDatabase