PYTHON-1309 cqlengine: Remove deepcopy on UserType deserialization#1192
PYTHON-1309 cqlengine: Remove deepcopy on UserType deserialization#1192k0machi wants to merge 1 commit intoapache:trunkfrom
Conversation
This change makes it so newly instanced UserType during deserialization isn't immediately copied by deepcopy, which could cause huge slowdown if that UserType contains a lot of data or nested UserTypes, in which case the deepcopy calls would cascade as each to_python call would eventually clone parts of source object. As there isn't a lot of information on why this deepcopy is here in the first place this change could potentially break something. Running integration tests against this commit does not produce regressions, so this call looks safe to remove, but I'm leaving this warning here for the future reference. Issue: scylladb#152
|
@absurdfarce you think someone would be able to take a look at this one ? (there was no response on the related issue: https://datastax-oss.atlassian.net/browse/PYTHON-1309) |
k0machi
changed the title
|
Thanks for the PR @k0machi! Apologies, this somehow completely slipped past me when it first came in. Have you signed the Contributor License Agreement for contributions to DataStax open source projects? If not you can find it at https://cla.datastax.com/. Thanks! |
@k0machi from my team in scylladb, a cla still needed ? Also we have this change for quite some time in our fork, no with issues (so far...) |
|
Ah, sorry, I didn't realize that @fruch ... if that's the case then no, we're good. |
Next time I'll tell him to be more vocal about where he's from 😜 |
3 participants
This change makes it so newly instanced UserType during deserialization isn't immediately copied by deepcopy, which could cause huge slowdown if that UserType contains a lot of data or nested UserTypes, in which case the deepcopy calls would cascade as each to_python call would eventually clone parts of source object. As there isn't a lot of information on why this deepcopy is here in the first place this change could potentially break something. Running integration tests against this commit does not produce regressions, so this call looks safe to remove, but I'm leaving this warning here for the future reference.
Issue: scylladb#152
Issue (upstream): PYTHON-1309
Here are flamegraphs for an application that uses cqlengine and encounters this issue:
Before
After
The request time goes from 10 seconds in this case down to 750ms, for reference the table is configured with a list of UserTypes that have a list of another UserType inside them.