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

Use orjson + encoder #1876

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Use orjson + encoder #1876

wants to merge 26 commits into from

Conversation

dsblank
Copy link
Member

@dsblank dsblank commented Jan 26, 2025

This PR uses orjson with a hand-coded encoder/decoder everywhere, and assumes orjson is installed.

Tests shows that this is 60% faster than master for db.iter_people() on a 40k person table.

@Nick-Hall
Copy link
Member

I think that we should make orjson mandatory. It would simplify the code and there seems no good reason not to do so. We are already going to include it in the Windows AIO and Mac bundle, and I have checked that all major Linux distributions include it.

@dsblank
Copy link
Member Author

dsblank commented Jan 26, 2025

I think that we should make orjson mandatory.

Yeah, I think you are right. Now that we can use it for all JSON load/dump, and it makes a big difference, it makes sense.

@dsblank
Copy link
Member Author

dsblank commented Jan 26, 2025

I think we can make it mandatory, but still have a fallback to regular json.loads/dumps. That way if someone runs Gramps on some weird architecture (or orjson fails for some reason), it will still have access to your data. That seems prudent with genealogical data for the future.

@Nick-Hall
Copy link
Member

By keeping both json and orjson you are adding extra code to maintain and extra tests to run for very little gain.

@dsblank
Copy link
Member Author

dsblank commented Jan 26, 2025

By keeping both json and orjson you are adding extra code to maintain and extra tests to run for very little gain.

@Nick-Hall, it's up to you. It was useful during testing to make sure everything is correct, but I can remove it now that all tests are passing. Let me know if you want me to remove the json.loads/dumps and turn the warning into an error. Removed.

The next step is to remove to_json, from_json, to_dict, from_dict throughout the code base. Do you want me to do it in this PR, or a follow-up one?

@dsblank dsblank marked this pull request as ready for review January 27, 2025 15:15
@dsblank
Copy link
Member Author

dsblank commented Jan 27, 2025

@Nick-Hall : Unable to locate package python3-orjson I think that is why I pip installed it.

@Nick-Hall
Copy link
Member

OK. It looks like python3-orjson is only available on Ubuntu 24.10 and later.

@Nick-Hall Nick-Hall added this to the v6.0 milestone Jan 27, 2025
@dsblank
Copy link
Member Author

dsblank commented Jan 28, 2025

@Nick-Hall, I have a refactor for this PR, and to fix usage across the code base. Do you want me to make a PR on this PR, or add it to this PR?

@Nick-Hall
Copy link
Member

Please add it to this PR.

@Nick-Hall
Copy link
Member

@dsblank Thanks. Now we can review all the changes together as a whole.

Please remember to add new files to either po/POTFILES.in or po/POTFILES.skip.

@dsblank
Copy link
Member Author

dsblank commented Jan 28, 2025

I think this is complete. I'll continue testing by using it

@kulath
Copy link
Member

kulath commented Jan 30, 2025

I don't think that methods like data_to_object should optionally omit the obj_class parameter. As far as I can see, in most cases the object class is known when the method is called.

    def data_to_object(obj_class, data):
    def data_to_object(data, obj_class=None):
        LOG.debug(
            "json, data_to_object: {'_class': %r, ...}",
            data["_class"] if (data and "_class" in data) else data,
        )
        return data_to_object(data)

(I don't understand how the call In the return in the above method is resolved, presumably to the data_to_object imported from utils, but that is probably just my ignorance)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants