-
Notifications
You must be signed in to change notification settings - Fork 60
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
Change serde::Serialize implementations to serialize structures directly without building up a JsonObject #152
Comments
Spawned from this discussion: #151 (comment) |
I'm interested in looking into this. It looks like flattening the map from |
I'm not very familiar with serde. It seems like this would also be a bunch of allocation? I was hoping for some kind of "visitor" pattern which avoid allocating anything per-geometry, but I don't know what's possible. |
maybe relevant: https://serde.rs/stream-array.html |
As described in <georust#152>, serialization by constructing a JsonObject has to essentially clone all data, which requires extra allocations. After this change, we pass data directly to the `Serializer`. `JsonObject` and `JsonValue` conversion impls were rewritten to delegate to `Serialize`. This requires some unfortunate `panic`s where we expect `Serialize` to produce a JSON object, but I think it's better than duplicating the serialization logic.
* Implement `Serialize` without going through JsonObject As described in <#152>, serialization by constructing a JsonObject has to essentially clone all data, which requires extra allocations. After this change, we pass data directly to the `Serializer`. `JsonObject` and `JsonValue` conversion impls were rewritten to delegate to `Serialize`. This requires some unfortunate `panic`s where we expect `Serialize` to produce a JSON object, but I think it's better than duplicating the serialization logic. * Serialize None properties as null * Add comments around to_value unwraps and panics
Currently, our
serde::Serialize
implementations build up aJsonObject
(aserde_json::Map
) that gets serialized by Serde, and then gets thrown away. Building up aJsonObject
causes new allocations to happen, including cloning all coordinates.Instead of building up a
JsonObject
, we should be able to use the serialization methods provided byserde::Serialize
directly. For reference, here's theserde::Serialize
implementation forserde_json::Value
which may be helpful. In particular, we'll probably want to build up ause serde::ser::SerializeMap
.After it's done, we should benchmark the performance. We currently only have a parsing benchmark, so we should create a new one for writing GeoJSON.
The text was updated successfully, but these errors were encountered: