Skip to content
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

Open
frewsxcv opened this issue Dec 3, 2020 · 4 comments

Comments

@frewsxcv
Copy link
Member

frewsxcv commented Dec 3, 2020

Currently, our serde::Serialize implementations build up a JsonObject (a serde_json::Map) that gets serialized by Serde, and then gets thrown away. Building up a JsonObject 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 by serde::Serialize directly. For reference, here's the serde::Serialize implementation for serde_json::Value which may be helpful. In particular, we'll probably want to build up a use 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.

@frewsxcv
Copy link
Member Author

frewsxcv commented Dec 3, 2020

Spawned from this discussion: #151 (comment)

@pjsier
Copy link
Member

pjsier commented Dec 8, 2020

I'm interested in looking into this. It looks like flattening the map from Value into Geometry might be a bit tricky, but I can open up an initial PR for feedback on that

@michaelkirk
Copy link
Member

michaelkirk commented Dec 8, 2020

want to build up a use serde::ser::SerializeMap

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.

@michaelkirk
Copy link
Member

maybe relevant: https://serde.rs/stream-array.html

zyla added a commit to zyla/geojson that referenced this issue Sep 20, 2023
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.
github-merge-queue bot pushed a commit that referenced this issue Sep 20, 2023
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants