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

Large optimization to unpickler #340

Merged
merged 3 commits into from Jan 17, 2021
Merged

Large optimization to unpickler #340

merged 3 commits into from Jan 17, 2021

Conversation

Theelx
Copy link
Contributor

@Theelx Theelx commented Jan 16, 2021

I realized that in certain cases, actually a sizeable number of cases, we don't need to check if it has a tag because it's a float, or an int, or something else that can't contain a string tag. This doubles the speed of unpickling for my personal use case (which involves a large amount of ints/floats), and should provide a statistically significant speed increase for most other cases also.

davvid added a commit that referenced this pull request Jan 17, 2021
Start the v1.5.1 cycle with a really nice performance bump.

Signed-off-by: David Aguilar <davvid@gmail.com>
@davvid davvid merged commit e588a19 into jsonpickle:master Jan 17, 2021
@davvid
Copy link
Member

davvid commented Jan 17, 2021

Sweet, thanks!

@Theelx
Copy link
Contributor Author

Theelx commented Jan 17, 2021

No problem! I was looking over encoder optimizations recently, do you think I should focus more on optimizing encoding or decoding?

@davvid
Copy link
Member

davvid commented Jan 17, 2021

Hmm that's a great question. I'm not really sure which has the most low-hanging fruit. decode() performance might benefit more users would be my guess. For example, an app that stores state may read it more often then it stores it, but like most things it's fairly app specific.

There's a bunch of backwards-compatibility code around the self._namestack, self._namedict and self._refname() in support of the older tags.REF stuff that was emitted by much older versions. There's already enough stuff since v1.5.0 that we can tag v1.5.1 shortly to clear the way for another minor (v1.6.0) that drops that backwards-compatibility support for tags.REF if it improves performance even further.

@Theelx
Copy link
Contributor Author

Theelx commented Jan 17, 2021

Just profiled that backwards-compatibility code, it doesn't seem to take up much cpu time or memory so I think I'll leave it for now.

Edit: We should also consider that according to SemVer 2.0, we can only break backwards compatibility when incrementing major versions, so 2.0.0 would be more appropriate if we were to take out support for tags.REF.

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.

None yet

2 participants