Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Remove manys calls to cursor_to_dict #16431

Merged
merged 5 commits into from
Oct 5, 2023
Merged

Conversation

clokep
Copy link
Member

@clokep clokep commented Oct 5, 2023

Instead of calling cursor_to_dict and then immediately unpacking the dict / changing the form to something else we should just use the raw tuples returned from the cursor.

A smoke test of this reduced my sync (for a rather small account on my test server) from 2757 allocations to 1861. Reducing the total allocated memory by _wait_for_sync_from_user from 217.2 KiB to 146.7 KiB.

@clokep
Copy link
Member Author

clokep commented Oct 5, 2023

(Also I secretly hate cursor_to_dict because it just hides types.)

@@ -101,7 +101,7 @@
class PusherConfig:
"""Parameters necessary to configure a pusher."""

id: Optional[str]
id: Optional[int]
Copy link
Member Author

Choose a reason for hiding this comment

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

This has had the wrong type forever, but was consistent with itself. Now that we're pulling the proper type from the database layer we had to update it here too.

Comment on lines 203 to 218
return (
TaskSchedulerWorkerStore._convert_row_to_task(
(
row["id"],
row["action"],
row["status"],
row["timestamp"],
row["resouce_id"],
row["params"],
row["result"],
row["error"],
)
)
if row
else None
)
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm hoping to convert simple_select_* methods too, but that's a bigger job. So for now we have this ugliness.

row["result"] = db_to_json(row["result"])
return ScheduledTask(**row)
def _convert_row_to_task(row: ScheduledTaskRow) -> ScheduledTask:
task_id, action, status, timestamp, resource_id, params, result, error = row
Copy link
Member Author

@clokep clokep Oct 5, 2023

Choose a reason for hiding this comment

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

Alternately we could just do row[0], etc.

This comment applies in a bunch of spots, let me know which style you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

does task_id, *rest = row work? or even task_id, *_ = row?

Copy link
Contributor

Choose a reason for hiding this comment

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

Those help readability, but from a quick test it would allocate another list:

>>> first, *rest = (1, 2, 3, 4)
>>> first
1
>>> rest
[2, 3, 4]
>>> first, *_ = (1, 2, 3, 4)
>>> _
[2, 3, 4]

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry if I'm unclear, I mean below this line of code we could do

ScheduledTaskRow(
    task_id=row[0],
    action=row[1],
    ...
)

Copy link
Member Author

Choose a reason for hiding this comment

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

(If there's no particular opinion I'll just merge this.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see. No strong opinions here.

DMRobertson
DMRobertson previously approved these changes Oct 5, 2023
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Are there any other uses of cursor_to_dict? Should we mark it as deprecated?

synapse/push/__init__.py Show resolved Hide resolved
synapse/storage/databases/main/presence.py Show resolved Hide resolved
synapse/storage/databases/main/pusher.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/pusher.py Show resolved Hide resolved
synapse/storage/databases/main/pusher.py Outdated Show resolved Hide resolved
synapse/storage/databases/main/room.py Show resolved Hide resolved
@DMRobertson
Copy link
Contributor

Err I'm a bit scared about #16431 (comment) actually

@DMRobertson DMRobertson dismissed their stale review October 5, 2023 13:01

forgot about the SELECT *

@clokep
Copy link
Member Author

clokep commented Oct 5, 2023

Are there any other uses of cursor_to_dict? Should we mark it as deprecated?

Yes, I plan to poke at more.

@clokep clokep marked this pull request as ready for review October 5, 2023 13:52
@clokep clokep requested a review from a team as a code owner October 5, 2023 13:52
@clokep clokep requested review from DMRobertson and removed request for a team October 5, 2023 13:52
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

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

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants