Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optimisation to StreamChangeCache #17130

Merged
merged 2 commits into from
May 6, 2024

Conversation

erikjohnston
Copy link
Member

@erikjohnston erikjohnston commented Apr 26, 2024

When there have been lots of changes compared with the number of entities, we can do a fast(er) path.

Locally I ran some benchmarking, and the comparison seems to give the best determination of which method we use.

When there have been lots of changes compared with the number of
entities, we can do a fast(er) path.
@erikjohnston erikjohnston marked this pull request as ready for review April 26, 2024 15:32
@erikjohnston erikjohnston requested a review from a team as a code owner April 26, 2024 15:32
Comment on lines +193 to +195
# If there have been tonnes of changes compared with the number of
# entities, it is faster to check each entities stream ordering
# one-by-one.
Copy link
Member

Choose a reason for hiding this comment

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

This conditional is more "if there have been more changes compared with the number of entities", given the _perf_factor during runtime is 1, right? In the case that the number of entities vs. changes is roughly the same, is it still the same speed or faster to make this calculation rather than what we had before?

If not, should we set the default of _perf_factor to something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I did a bunch of testing to figure out at what point it's faster, and it is indeed with a _perf_factor of 1.

I think that intuitively makes some sense: get_all_entities_changed essentially involves one look up per stream position, and checking each entity one by one requires one lookup per entity.

Copy link
Member

Choose a reason for hiding this comment

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

Right, sounds plausible!

@erikjohnston erikjohnston requested a review from a team May 3, 2024 15:32
return {
entity
for entity in entities
if self._entity_to_key.get(entity, -1) > stream_pos
Copy link
Member

Choose a reason for hiding this comment

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

I seem to remember that stream_pos can go negative... is that relevant here?

Copy link
Member Author

Choose a reason for hiding this comment

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

StreamChangeCache assumes they're positive incrementing numbers. The only stream that has negatives is the backfill stream that goes backwards (though in helper classes tend to be modelled as positive numbers), which doesn't have a StreamChangeCache.

@erikjohnston erikjohnston merged commit 3e6ee8f into develop May 6, 2024
38 checks passed
@erikjohnston erikjohnston deleted the erikj/fast_path_stream_change_cache branch May 6, 2024 11:56
yingziwu added a commit to yingziwu/synapse that referenced this pull request May 15, 2024
Synapse 1.107.0 (2024-05-14)
============================

No significant changes since 1.107.0rc1.

- Add preliminary support for [MSC3823: Account Suspension](matrix-org/matrix-spec-proposals#3823). ([\#17051](element-hq/synapse#17051))
- Declare support for [Matrix v1.10](https://matrix.org/blog/2024/03/22/matrix-v1.10-release/). Contributed by @clokep. ([\#17082](element-hq/synapse#17082))
- Add support for [MSC4115: membership metadata on events](matrix-org/matrix-spec-proposals#4115). ([\#17104](element-hq/synapse#17104), [\#17137](element-hq/synapse#17137))

- Fixed search feature of Element Android on homesevers using SQLite by returning search terms as search highlights. ([\#17000](element-hq/synapse#17000))
- Fixes a bug introduced in v1.52.0 where the `destination` query parameter for the  [Destination Rooms Admin API](https://element-hq.github.io/synapse/v1.105/usage/administration/admin_api/federation.html#destination-rooms) failed to actually filter returned rooms. ([\#17077](element-hq/synapse#17077))
- For MSC3266 room summaries, support queries at the recommended endpoint of `/_matrix/client/unstable/im.nheko.summary/summary/{roomIdOrAlias}`. The existing endpoint of `/_matrix/client/unstable/im.nheko.summary/rooms/{roomIdOrAlias}/summary` is deprecated. ([\#17078](element-hq/synapse#17078))
- Apply user email & picture during OIDC registration if present & selected. ([\#17120](element-hq/synapse#17120))
- Improve error message for cross signing reset with [MSC3861](matrix-org/matrix-spec-proposals#3861) enabled. ([\#17121](element-hq/synapse#17121))
- Fix a bug which meant that to-device messages received over federation could be dropped when the server was under load or networking problems caused problems between Synapse processes or the database. ([\#17127](element-hq/synapse#17127))
- Fix bug where `StreamChangeCache` would not respect configured cache factors. ([\#17152](element-hq/synapse#17152))

- Correct licensing metadata on Docker image. ([\#17141](element-hq/synapse#17141))

- Update the `event_cache_size` and `global_factor` configuration options' documentation. ([\#17071](element-hq/synapse#17071))
- Remove broken sphinx docs. ([\#17073](element-hq/synapse#17073), [\#17148](element-hq/synapse#17148))
- Add RuntimeDirectory to example matrix-synapse.service systemd unit. ([\#17084](element-hq/synapse#17084))
- Fix various small typos throughout the docs. ([\#17114](element-hq/synapse#17114))
- Update enable_notifs configuration documentation. ([\#17116](element-hq/synapse#17116))
- Update the Upgrade Notes with the latest minimum supported Rust version of 1.66.0. Contributed by @jahway603. ([\#17140](element-hq/synapse#17140))

- Enable [MSC3266](matrix-org/matrix-spec-proposals#3266) by default in the Synapse Complement image. ([\#17105](element-hq/synapse#17105))
- Add optimisation to `StreamChangeCache.get_entities_changed(..)`. ([\#17130](element-hq/synapse#17130))

* Bump furo from 2024.1.29 to 2024.4.27. ([\#17133](element-hq/synapse#17133))
* Bump idna from 3.6 to 3.7. ([\#17136](element-hq/synapse#17136))
* Bump jsonschema from 4.21.1 to 4.22.0. ([\#17157](element-hq/synapse#17157))
* Bump lxml from 5.1.0 to 5.2.1. ([\#17158](element-hq/synapse#17158))
* Bump phonenumbers from 8.13.29 to 8.13.35. ([\#17106](element-hq/synapse#17106))
- Bump pillow from 10.2.0 to 10.3.0. ([\#17146](element-hq/synapse#17146))
* Bump pydantic from 2.6.4 to 2.7.0. ([\#17107](element-hq/synapse#17107))
* Bump pydantic from 2.7.0 to 2.7.1. ([\#17160](element-hq/synapse#17160))
* Bump pyicu from 2.12 to 2.13. ([\#17109](element-hq/synapse#17109))
* Bump serde from 1.0.197 to 1.0.198. ([\#17111](element-hq/synapse#17111))
* Bump serde from 1.0.198 to 1.0.199. ([\#17132](element-hq/synapse#17132))
* Bump serde from 1.0.199 to 1.0.200. ([\#17161](element-hq/synapse#17161))
* Bump serde_json from 1.0.115 to 1.0.116. ([\#17112](element-hq/synapse#17112))
- Update `tornado` Python dependency from 6.2 to 6.4. ([\#17131](element-hq/synapse#17131))
* Bump twisted from 23.10.0 to 24.3.0. ([\#17135](element-hq/synapse#17135))
* Bump types-bleach from 6.1.0.1 to 6.1.0.20240331. ([\#17110](element-hq/synapse#17110))
* Bump types-pillow from 10.2.0.20240415 to 10.2.0.20240423. ([\#17159](element-hq/synapse#17159))
* Bump types-setuptools from 69.0.0.20240125 to 69.5.0.20240423. ([\#17134](element-hq/synapse#17134))
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.

2 participants