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

Avoid unnecessary work in the spaces summary. #10085

Closed
wants to merge 9 commits into from

Conversation

clokep
Copy link
Member

@clokep clokep commented May 27, 2021

This has a few different changes in separate commits:

  • Only search for m.child.space events in spaces, not all rooms.
  • Do not track event pairs -- I think this will break some behavior with Dendrite, but Dendrite doesn't return the proper result at the moment anyway, so I'm not too concerned about this.
  • Do not include events which point to inaccessible rooms (or are otherwise not part of the graph).

The first and the third bullets are driven by doing work that the clients drop on the floor (they do not show a normal room as a space and do not care about events with not matching room). The MSC was updated with these changes.

My hope is that this will help servers which have a slow database (or are starved for CPU, maybe). There's been some reports of the endpoint being very slow.

There is a corresponding complement PR: matrix-org/complement#123.

This depends on #10160 being merged first.

This is also some prep work for handling pagination in a roundabout way.

@clokep
Copy link
Member Author

clokep commented May 27, 2021

This is going to fail complement tests since that depends on the old behavior, I'm not really sure how to get complement and synapse on the same page in this instance. 😢

@clokep clokep force-pushed the clokep/spaces-simplify branch 3 times, most recently from d05fb66 to 1cae18a Compare June 10, 2021 11:35
@clokep clokep changed the title Do not recurse into rooms that are not spaces. Avoid unnecessary work in the spaces summary. Jun 10, 2021
synapse/api/constants.py Outdated Show resolved Hide resolved
@clokep clokep marked this pull request as ready for review June 11, 2021 13:41
@clokep clokep requested a review from a team June 11, 2021 13:42
@richvdh richvdh self-assigned this Jun 14, 2021
synapse/api/constants.py Show resolved Hide resolved
synapse/handlers/space_summary.py Outdated Show resolved Hide resolved
synapse/handlers/space_summary.py Outdated Show resolved Hide resolved
synapse/handlers/space_summary.py Outdated Show resolved Hide resolved
synapse/handlers/space_summary.py Outdated Show resolved Hide resolved
synapse/handlers/space_summary.py Show resolved Hide resolved
synapse/handlers/space_summary.py Outdated Show resolved Hide resolved
synapse/handlers/space_summary.py Outdated Show resolved Hide resolved
synapse/handlers/space_summary.py Outdated Show resolved Hide resolved
Comment on lines 221 to 223
# Note that events are pending of the child event since if
# the parent event was not accessible it shouldn't be included
# at all.
Copy link
Member

Choose a reason for hiding this comment

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

I don't really follow this logic. Why do we only add the event to the pending list for the child room?

Copy link
Member Author

Choose a reason for hiding this comment

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

The thought process was that we only want to send the event in the case of both the parent and child room being accessible to the requester.

I think the assumption here is that if the parent room isn't in processed_rooms something terrible has happened? There are two sources of data we're iterating over:

  • events - m.space.child events from the current room.
    • For local rooms, the parent will be the current room, the child will be a room that may or may not have been processed.
    • For remote rooms, this includes the above, as well as additional events that might be children of children, etc. (Or even events that aren't part of the graph.)
  • room_pending_events - m.space.child events which were previously found to be pointing to the current room, i.e. the child room is the current room and the parent room is a previously processed room.

I'm now having trouble convincing myself that parents must be processed before children and I think that assumption might be bogus actually.

@clokep
Copy link
Member Author

clokep commented Jun 18, 2021

I'm going to merge #10208 into this and build out some more tests to ensure this is working as I expect.

@clokep
Copy link
Member Author

clokep commented Jun 29, 2021

I'm going to abandon this and re-do this work.

@clokep clokep closed this Jun 29, 2021
@clokep clokep deleted the clokep/spaces-simplify branch July 29, 2021 13:01
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