Initialise: make snapshots instead of prepending state events #310
Conversation
DMRobertson
force-pushed
the
dmr/resnapshot
branch
from
bb0d463
to
4b9b98a
Compare
DMRobertson
changed the title
| func (s *stateMap) Ingest(e Event) (replacedNID int64) { | ||
| if e.Type == "m.room.member" { | ||
| replacedNID = s.Memberships[e.StateKey] | ||
| s.Memberships[e.StateKey] = e.NID | ||
| } else { | ||
| key := [2]string{e.Type, e.StateKey} | ||
| replacedNID = s.Other[key] | ||
| s.Other[key] = e.NID | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
I don't actually use the return value here. At some point I was more ambitious and wanted to reuse this machinery in Accumulate... but then I decided that was not really getting us anything and left it for a rainy day.
| func (s *stateMap) NIDs() (membershipNIDs, otherNIDs []int64) { | ||
| membershipNIDs = make([]int64, 0, len(s.Memberships)) | ||
| otherNIDs = make([]int64, 0, len(s.Other)) | ||
| for _, nid := range s.Memberships { | ||
| membershipNIDs = append(membershipNIDs, nid) | ||
| } | ||
| for _, nid := range s.Other { | ||
| otherNIDs = append(otherNIDs, nid) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
It feels a little inefficient to convert from []Event to maps and then back to []int64 again if we're only changing the state by one event. But we're optimising for leaving and rejoining large rooms here, with large gappy state blocks. We've seen gappy state blocks of size multiple thousands on the m.org deployment!
| // pull stripped events as this may be huge (think Matrix HQ) | ||
| events, err := a.eventsTable.SelectStrippedEventsByNIDs(txn, true, append(snapshot.MembershipEvents, snapshot.OtherEvents...)) | ||
| if err != nil { | ||
| return stateMap{}, err |
There was a problem hiding this comment.
We only want event NID, type and state key here, so we could use a more specialised query here to fetch less data.
| syncResp = alice.SlidingSync(t, | ||
| sync3.Request{ | ||
| Lists: map[string]sync3.RequestList{ | ||
| "a": { | ||
| Ranges: [][2]int64{{0, 20}}, | ||
| RoomSubscription: sync3.RoomSubscription{ | ||
| TimelineLimit: 10, | ||
| TimelineLimit: 100, |
There was a problem hiding this comment.
This test does produce a limited sync for the poller. But that poll is for a new device so gets timelimit limit 1 which wasn't satisfactory---I wanted to confirm that we include timeline events after the gap, but not before.
I felt it was easier to write an integration test so I can use the same poller without any logging out and in shenanigans.
state/accumulator.go
Outdated
| // Update the map from (2) with the events in in `state`. (There must be similar logic already in A ccumulate for this?) | ||
| // Store the snapshot. Mark the room's current state as being this snapshot. | ||
| // | ||
| // 5. If the starting snapshot ID was not zero, emit a cache invalidation payload. |
There was a problem hiding this comment.
I seem to have forgotten to do this... but the room name changes are seen in the tests. Does that mean the cache invalidation payload isn't needed?
There was a problem hiding this comment.
Those tests are only testing cache invalidation via redaction, not cache invalidation via joining a room after a long period of no one being in the room. The latter case is what we're trying to catch here.
There was a problem hiding this comment.
I don't follow. I specifically mean TestGappyState and TestGappyStateDoesNotAccumulateTheStateBlock, neither of which emit a redaction event.
state/accumulator.go
Outdated
| // Update the map from (2) with the events in in `state`. (There must be similar logic already in A ccumulate for this?) | ||
| // Store the snapshot. Mark the room's current state as being this snapshot. | ||
| // | ||
| // 5. If the starting snapshot ID was not zero, emit a cache invalidation payload. |
There was a problem hiding this comment.
Those tests are only testing cache invalidation via redaction, not cache invalidation via joining a room after a long period of no one being in the room. The latter case is what we're trying to catch here.
Co-authored-by: kegsay <kegan@matrix.org>
I want to write something that convinces me the joined user tracker is updated after the cache invalidation msg
DMRobertson
force-pushed
the
dmr/resnapshot
branch
from
a3c3a08
to
9bc2962
Compare
hope was that the "has room changed" logic would kick in
|
I had to rethink a lot of this. -> #329. |
Primarily a perf fix for when the proxy re-joins a room, but it's also arguably a correctness fix because we have no idea how these things appear in the timeline.