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

Simplify shared union serializer logic #1538

Merged
merged 9 commits into from
Nov 13, 2024
Merged

Conversation

sydney-runkle
Copy link
Member

No description provided.

Copy link

codspeed-hq bot commented Nov 11, 2024

CodSpeed Performance Report

Merging #1538 will not alter performance

Comparing dh/wip-union-simplification (ee9dd3d) with main (061711f)

Summary

✅ 155 untouched benchmarks

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looking great! I think we can go a bit further for fun and profit 😄

src/serializers/type_serializers/union.rs Show resolved Hide resolved
src/serializers/type_serializers/union.rs Outdated Show resolved Hide resolved
@sydney-runkle
Copy link
Member Author

Failing integration test is unrelated to this PR, should be resolved soon once we merge pydantic/pydantic#10825 into main on pydantic

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

This looks like a solid improvement to me! 🎉

@sydney-runkle sydney-runkle merged commit 6472887 into main Nov 13, 2024
29 checks passed
@sydney-runkle sydney-runkle deleted the dh/wip-union-simplification branch November 13, 2024 16:00
@sydney-runkle sydney-runkle changed the title WIP: Simplify shared union serializer logic Simplify shared union serializer logic Nov 13, 2024
@MarkusSintonen
Copy link
Contributor

MarkusSintonen commented Nov 14, 2024

@sydney-runkle did you try running CodSpeed in Pydantic side for this PR? I accidentally rebased my changes on top of pydantic-core master (instead of release tag) and pointed Pydantic side PR to point into the master based code (which included only this PR changes). CodSpeed showed this which is quite a regression:

❌ test_north_star_dump_json 31.7 ms 57.9 ms -45.26%

@davidhewitt
Copy link
Contributor

I think the benchmarks we have here should have identified if this PR was a problem, but it's not impossible it could slip through.

@davidhewitt
Copy link
Contributor

This PR should in theory have no functional change so definitely that order of magnitude seems incorrect.

@MarkusSintonen
Copy link
Contributor

@davidhewitt / @sydney-runkle it is still showing the exactly same here: pydantic/pydantic#10845 🤔 Weird...

@sydney-runkle
Copy link
Member Author

Huh, that is really odd. @davidhewitt, I might defer to you here - not sure why our changes here would degrade perf...

include,
exclude,
match tagged_union_serialize(
None,
Copy link
Contributor

Choose a reason for hiding this comment

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

@sydney-runkle this is the source of the perf regression; accidentally switched off tagged union serialization optimization in the JSON case.

Copy link
Member Author

Choose a reason for hiding this comment

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

😬 oops. Great find!

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