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

Implement Deserializer for Pod #5

Open
kmaasrud opened this issue Sep 6, 2021 · 4 comments
Open

Implement Deserializer for Pod #5

kmaasrud opened this issue Sep 6, 2021 · 4 comments
Labels
enhancement New feature or request performance

Comments

@kmaasrud
Copy link
Collaborator

kmaasrud commented Sep 6, 2021

While investigating #3, I realised that we don't really have a need for the polyglot datatype Pod. Since we nonetheless coerce it into a serde_json::Value upon deserializing, Pod can easily be replaced by Value - essentially skipping a conversion step.

This would increase performance (probably not by much, but every nanosecond counts) and drastically reduce code complexity.

Changing this would introduce an API change, so I don't see the rush to do it, but I'm leaving this issue here for whenever we deem it fit to implement.

@yuchanns
Copy link
Collaborator

yuchanns commented Sep 6, 2021

Well, when I wrote this crate, I was expecting a scenario where the frontmatter of markdown is parsed out at compile time so that yew can generate static files, which is similar to what vuepress does. So I didn't care about performance, since it's a compile-time overhead.

However, I have noticed that some people use it at runtime.

So I general totally agree with you that a little bit of runtime performance optimization is needed.

But as to whether discarding pod types to improve performance, some benchmark comparing this two way shall be done to make it more convincing.

When the pod type was introduced into the crate, I hadn't considered serd_json so I didn't use value instead.

@yuchanns
Copy link
Collaborator

yuchanns commented Sep 6, 2021

Also, I think we should do optimization as soon as it is proven necessary. The fewer users, the less cost to make breaking changes. Just follow semantics well.

@kmaasrud
Copy link
Collaborator Author

kmaasrud commented Sep 6, 2021

Thinking more about it, perhaps we should keep Pod after all! It's not a bad thing to be in control of our own datatype (if we for instance want to extend into supporting TOML's datetimes).

The deserialize function is essentially what needs to be changed. Namely, we need to implement Deserialize on Pod to avoid the need for a conversion into serde_json::Value. This wouldn't introduce an API change either 😄

@kmaasrud kmaasrud changed the title No real need for Pod datatype Implement Deserialize for Pod Sep 6, 2021
@yuchanns
Copy link
Collaborator

yuchanns commented Sep 6, 2021

Yes, we should think about this a bit more carefully. Currently using serde_json to do serialization and deserialization is just for the sake of meet my need quickly, so I didn't bother with that part, it's a bit complicated after all. Since runtime performance is a concern, it's time to do this part ourselves

@yuchanns yuchanns added the enhancement New feature or request label Sep 6, 2021
@kmaasrud kmaasrud changed the title Implement Deserialize for Pod Implement Deserializer for Pod Sep 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance
Projects
None yet
Development

No branches or pull requests

2 participants