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

Add an API to choose the underlying JSON encoder. #25

Merged
merged 6 commits into from
Jul 27, 2020

Conversation

clokep
Copy link
Member

@clokep clokep commented Jul 23, 2020

This is an alternative to #24, which allows for better backwards compatibility for a period of time. It lets libraries depending on canonicaljson opt into using a different JSON implementation.

canonicaljson.py Outdated Show resolved Hide resolved
@clokep clokep marked this pull request as draft July 23, 2020 14:56
@clokep
Copy link
Member Author

clokep commented Jul 23, 2020

See #26 for the failing builds.

@clokep
Copy link
Member Author

clokep commented Jul 23, 2020

This is probably ready to go, minus some CI issues.

@clokep clokep marked this pull request as ready for review July 24, 2020 16:57
@clokep clokep requested a review from a team July 24, 2020 16:58
@clokep
Copy link
Member Author

clokep commented Jul 24, 2020

See matrix-org/synapse#7674 for how this would be used.

Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

I'm a bit confused, as we seem to use simplejson in this library because we think it better performant than stdlib json. Yet we're adding functionality to allow using json as we in Synapse think it's more performant than simplejson?

I'm guessing we're doing this so that the library implementation won't change from under other people using canonicaljson? Could we do that via a version bump of canonicaljson instead?

canonicaljson.py Outdated Show resolved Hide resolved
@clokep
Copy link
Member Author

clokep commented Jul 24, 2020

@anoadragon453 There's a bunch of conversation about this in matrix-org/synapse#7674 (comment), but the gist is:

  • Synapse doesn't put a max version on the canonicaljson dependency, so any change must be backwards compatible to not break a new install of an old Synapse.
  • The goal of this is to allow Synapse to NOT use simplejson, in a backwards compatible fashion. The approach taken here is that it is backwards compatible unless you opt into new behavior.

as we seem to use simplejson in this library because we think it better performant than stdlib json

We used to think this, the latest profiling done shows this to no longer be true.

Let me know if that clears things up or not!

Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
@anoadragon453
Copy link
Member

Ahh, I see now. We're not only worrying about other users of this library, but also old versions of Synapse. Makes sense 🙂

as we seem to use simplejson in this library because we think it better performant than stdlib json

We used to think this, the latest profiling done shows this to no longer be true.

Yes, I was thinking we should just change the default in this library to always be stdlib json based on the new benchmark data, but I'm guessing that would be a backwards incompatible change.

@clokep
Copy link
Member Author

clokep commented Jul 24, 2020

but I'm guessing that would be a backwards incompatible change.

The standard library JSON on Python 3.5 doesn't handle bytes well, so...yeah it isn't backwards compatible. Hopefully we can do that at some point in the future!

Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

That clears things up then. Thanks!

@clokep clokep merged commit 225140e into master Jul 27, 2020
@clokep clokep deleted the clokep/chooseable-json branch July 27, 2020 13:52
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