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

Extend compatibility to C implementation of frozendict #37

Merged
merged 2 commits into from
Sep 28, 2021

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Sep 23, 2021

Fixes issue #36.
Background: some versions of frozendict contain a C implementation of frozendict that was incompatible with our code. This PR adds logic to catch and handle those cases.

canonicaljson.py Outdated Show resolved Hide resolved
@reivilibre
Copy link
Contributor

I wonder if we should be checking frozendict.c_ext and choosing what to do based off of that (though I guess that attribute doesn't exist on old versions, so we would probably have to check it exists [getattr(frozendict, "c_ext", False)?])?
The use of exceptions for control flow is one of those things that is usually slightly nasty, though Python doesn't seem to shy away from it as much, so maybe I'm being too sensitive here.

@reivilibre reivilibre requested a review from a team September 24, 2021 08:19
@H-Shay H-Shay changed the title Extend compatibility to Cpython implementation of frozendict Extend compatibility to C implementation of frozendict Sep 24, 2021
Co-authored-by: reivilibre <olivier@librepush.net>
@squahtx
Copy link
Contributor

squahtx commented Sep 28, 2021

I wonder if we should be checking frozendict.c_ext and choosing what to do based off of that (though I guess that attribute doesn't exist on old versions, so we would probably have to check it exists [getattr(frozendict, "c_ext", False)?])?
The use of exceptions for control flow is one of those things that is usually slightly nasty, though Python doesn't seem to shy away from it as much, so maybe I'm being too sensitive here.

I did some incredibly unscientific testing and saw that the try-except was faster than doing a getattr when no exception is thrown (frozendict 1.2) and slower when an exception is thrown (frozendict 2.0.6 with c_ext). A try-except which doesn't raise seems to be just as fast as not using a try-except. We could determine which implementation to use at startup of course, which would be the best of both worlds, albeit more code to maintain.

IMO the PR as it stands is a definite improvement to the code and shouldn't regress performance for any of our existing supported frozendict versions.

Copy link
Contributor

@reivilibre reivilibre 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 relatively happy to go with the above; it's hard to argue against the fact that it is a definite improvement (perhaps slow vs broken, with the existing case being pretty much unaffected).

We could determine which implementation to use at startup of course, which would be the best of both worlds, albeit more code to maintain.

For what it's worth, this seems like it would be a one-liner;

frozendict_using_c_extension = getattr(frozendict, "c_ext", False) and then using this in the if statement (if we're truly concerned about getattr performance — I'd be surprised if this was a particular concern because it should just be a hashtable lookup).

I feel a little bit bad about constructing a backtrace, throwing an exception and catching it for every frozendict we serialise, but it's not clear when the C extension is actually used (installing frozendict on my machine doesn't use it by default), and furthermore we're already resorting to copying the dict, which will already add its own performance overhead.

@squahtx
Copy link
Contributor

squahtx commented Sep 28, 2021

frozendict_using_c_extension = getattr(frozendict, "c_ext", False) and then using this in the if statement (if we're truly concerned about getattr performance — I'd be surprised if this was a particular concern because it should just be a hashtable lookup).

Ah, good point. I was thinking about lifting the if out of the function entirely, which would be a few lines extra.

@H-Shay H-Shay merged commit efea6c6 into matrix-org:main Sep 28, 2021
@DMRobertson DMRobertson linked an issue Sep 30, 2021 that may be closed by this pull request
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.

Tests fail with frozendict 2.0.6
4 participants