Speed bottleneck on deepcopy

[MicaelJarniac]

We're having issues with extremely slow queries while using the object mapper. The raw query itself isn't slow, but instancing the objects in Python seems to be.

While profiling, we noticed that there seems to be a bottleneck on the following line, using deepcopy:

python-driver/cassandra/cqlengine/columns.py

Line 1041 in 94b64bb

copied_value = deepcopy(value)

We're not entirely sure what that deepcopy is for, so we tried removing it locally, and it seemed to result in a huge speed boost.

I believe simply removing it might break something, so I won't suggest doing that straight away, but I believe there might be a better approach to this that could avoid deepcopy when not necessary.

[ultrabug]

FYI @jsurloppe @dargor @wyfo since we're into Python Scylla perf analysis: maybe we're affected somehow too and investigating could help?

@MicaelJarniac please be kindly reminded that we maintain a fork of the official cassandra driver here so we don't have all the background and history on every design and legacy decision

The plan of ScyllaDB is to switch out from this fork someday and use the scylla rust driver as a sane base for other language drivers with bindings.

[fruch]

I think this can easily be replaced by dict comprehension

        copied_value = {name:  field.to_python(value[name]) if value is not None or isinstance(field, BaseContainerColumn) else value[name] for name, field in self.user_type._fields.items() }
        return copied_value

[ultrabug]

@MicaelJarniac did you try what @fruch is proposing by any chance?

[wyfo]

I'm just wondering why is there or instead of and in

if copied_value[name] is not None or isinstance(field, BaseContainerColumn)

I mean, to_python seems to be a method of BaseContainerColumn, it's illogical to call it if only the first part of the test is true.

I also think there is a mistake in @fruch code because if value is not None should be if value[name] is not None.

I don't know this driver, so I don't know if there is some hidden implications for deepcopy removal; but it seems to me that it would be ok.
However, performance-wise, I would not use a dict-comprehension, but just replace deepcopy by a call to dict.copy.

$ python -m timeit -s "deepcopy = __import__(\"copy\").deepcopy; N = 10; value = dict(zip(range(N), range(N)))" "copied
 = deepcopy(value)" "for i in range(N):" "    if (val := value[i]) >= 5:" "        copied[i] = val + 1"
20000 loops, best of 5: 12.8 usec per loop
$ python -m timeit -s "N = 10; value = dict(zip(range(N), range(N)))" "{i: value[i] if value[i] < 5 else value[i] + 1 for i in range(N)}"
200000 loops, best of 5: 1.8 usec per loop
$ python -m timeit -s "N = 10; value = dict(zip(range(N), range(N)))" "copied = value.copy()" "for i in range(N):" "     if value[i] >= 5:" "        copied[i] = copied[i] + 1"
200000 loops, best of 5: 1.28 usec per loop

[fruch]

@MicaelJarniac @wyfo, since this part of the code isn't scylla specific,
if this change is that much impact, I would suggest taking it upstream to https://github.com/datastax/python-driver (so the rest of the community would gain from it)

[MicaelJarniac]

@MicaelJarniac @wyfo, since this part of the code isn't scylla specific, if this change is that much impact, I would suggest taking it upstream to https://github.com/datastax/python-driver (so the rest of the community would gain from it)

I was originally going to open this issue there, but they don't use GitHub Issues; they want us to create an account on another tracker or whatever, so I didn't bother to go through their bureaucracy.

But I agree, it'd make more sense for this to go there instead.

@MicaelJarniac did you try what @fruch is proposing by any chance?

I haven't tested it yet, sadly.

[MicaelJarniac]

https://datastax-oss.atlassian.net/browse/PYTHON-1309

[fruch]

@MicaelJarniac if you'll get a cold shoulder there, we could try applying those fixes here, but I'll first would want to enable the cqlengine integration tests, we are not really running them under this fork.

also if you guys some benchmark test you are using, it would be nice if you could share them (or even contribute them as tests)

[k0machi]

I've also encountered this issue with a Model that contains a UDT that contains a list of nested UDTs, this deepcopy call extends the query well into dozens of seconds in such a scenario. I see that upstream didn't take a look at the issue at all so maybe we should fix this issue on our end.

[k0machi]

After running cqlengine tests I have not found any regressions when completely removing the deepcopy in to_python. Removing deepcopy from to_database causes several tests to fail (as the source object now mutates when it is being serialized for the database.

I believe the rationale behind those deepcopies is to protect source object from modifications during db operations, however in to_python case I do not see a reason why it needs to be copied after being deserialized, so I think we may be able to remove it with no issues. In my tests I have found an almost 12x speedup when deserializing complex tables.

[fruch]

After running cqlengine tests I have not found any regressions when completely removing the deepcopy in to_python. Removing deepcopy from to_database causes several tests to fail (as the source object now mutates when it is being serialized for the database.

I believe the rationale behind those deepcopies is to protect source object from modifications during db operations, however in to_python case I do not see a reason why it needs to be copied after being deserialized, so I think we may be able to remove it with no issues. In my tests I have found an almost 12x speedup when deserializing complex tables.

Do a PR with this, and we'll put it on the next release