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

Minimal fix for unneccessary JSON AST creation #2

Merged
merged 2 commits into from
May 21, 2021

Conversation

benjamin-actyx
Copy link
Contributor

As #1 has stalled, here’s a working (I believe) fix that isn’t very pretty but works.

Copy link
Contributor

@rklaehn rklaehn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I was trying to use RawValue as well, but I ran into some issues.

serde_json::RawValue is buggy and needs to be used with care, as I unfortunately had to find out. But it seems this particular usage does not trigger the bug.

@rklaehn
Copy link
Contributor

rklaehn commented May 21, 2021

I think I tried to use RawValue in both incoming and outgoing, which triggered the bug.

Here is the bug: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=4d9015b013535f143aa5b8ad829be43c

Copy link
Contributor

@aruediger aruediger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just going to mention https://github.com/Actyx/Cosmos/issues/6202#issuecomment-824817563 as well. I assume you've tried it with the actyx API so 👍 .

@rklaehn
Copy link
Contributor

rklaehn commented May 21, 2021

I created an issue in serde, but I don't expect this to be fixed: serde-rs/json#779

The way serde is implemented it is basically impossible to implement something like RawValue. The serde_json RawValue is an extremely ugly hack which never really worked.

Makes your eyes bleed. I remember Endre complaining about this...
https://github.com/serde-rs/json/blob/master/src/raw.rs#L278

In libipld we got a proper RawValue....

@benjamin-actyx
Copy link
Contributor Author

But it seems this particular usage does not trigger the bug.

Hmm, what’s the decisive difference to your reproducer?

@rklaehn
Copy link
Contributor

rklaehn commented May 21, 2021

But it seems this particular usage does not trigger the bug.

Hmm, what’s the decisive difference to your reproducer?

I think you are only trying to write, not read. Writing is unproblematic. But that is also fine, since using serde_json::Value for incoming requests is probably fine. The performance problem is for outgoing responses, which can be a lot, right?

@benjamin-actyx
Copy link
Contributor Author

Aah, you are trying to deserialize 'directly' into a Box<RawValue>, and that does not work. I get it.

@benjamin-actyx benjamin-actyx merged commit 49199d0 into master May 21, 2021
@benjamin-actyx benjamin-actyx deleted the bsi/dont-serialize-twice branch May 21, 2021 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants