Skip to content

Conversation

grigi
Copy link
Contributor

@grigi grigi commented Jan 16, 2019

Original: Tests run in 87ms

newone = type(self)()
newone.__dict__.update(self.__dict__)
for key, value in self.__dict__.items():
    if isinstance(value, (set, list)):
        newone.__dict__[key] = type(value)(x for x in value)

Optimization 1: Tests run in 73ms (~19% speedup)

Use __new__ instead of __init__ because we are going to clobber its state
Use copy() (shallow copy) which is supported on both sets and lists and faster than a comprehension

newone = type(self).__new__(type(self))
newone.__dict__.update(self.__dict__)
for key, value in self.__dict__.items():
    if isinstance(value, (set, list)):
        newone.__dict__[key] = copy(value)

Optimization 2: Tests run in 67ms (30% speedup) INVALID

Unroll the loop

newone = type(self).__new__(type(self))
newone.__dict__.update(self.__dict__)
newone._select_star_tables = copy(self._select_star_tables)
newone._from = copy(self._from)
newone._with = copy(self._with)
newone._selects = copy(self._selects)
newone._columns = copy(self._columns)
newone._values = copy(self._values)
newone._groupbys = copy(self._groupbys)
newone._orderbys = copy(self._orderbys)
newone._joins = copy(self._joins)
newone._unions = copy(self._unions)
newone._updates = copy(self._updates)

Invalid due to subclasses do define extra lists. And the unrolled variant doesn't copy everything it needs.

Optimization 2b: Tests run in 67ms (~30% speedup)

Same as Opt2, but subclasses that define lists (MySQLQueryBuilder & PostgreQueryBuilder only)

newone = super(MySQLQueryBuilder, self).__copy__()
newone._duplicate_updates = copy(self._duplicate_updates)

For Opt 1:
When running the benchmark suite for Tortoise ORM, two of the benchmarks get a 6% and 9% performance increase respectively.

For Opt2b:
When running the benchmark suite for Tortoise ORM, two of the benchmarks get a 12% and 15% performance increase respectively.

Due to the perf increase for Opt2a being noticeably better than Opt1, I opted to go with that

I had to use copy.copy() instead of <list/set>.copy() for Py2.7 compatibility reasons.

@grigi grigi requested review from twheys and a team as code owners January 16, 2019 21:30
@coveralls
Copy link

coveralls commented Jan 16, 2019

Coverage Status

Coverage increased (+0.009%) to 98.129% when pulling 9a6daf2 on grigi:optimize_copy into 139c45a on kayak:master.

@grigi
Copy link
Contributor Author

grigi commented Jan 17, 2019

@twheys I fixed an oversight, it should be correct now.
Please review.

@twheys twheys merged commit ee73c32 into kayak:master Jan 23, 2019
@twheys twheys self-assigned this Jan 23, 2019
@twheys twheys added the 0.22.3 label Jan 23, 2019
@twheys
Copy link
Contributor

twheys commented Jan 23, 2019

Great 👍 Thanks! Merged and published.

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