Repository navigation
Some relatively mechanical cleanups - #154
Conversation
edaa8a6 to
cfbf08a
Compare
justinpettit
left a comment
justinpettit
left a comment
There was a problem hiding this comment.
Overall looks good. Just had one concern about holding a lock too long.
| let new_generation = mutable.messages.generation(); | ||
| if new_generation != generation { | ||
| assert!(new_generation > generation); | ||
| self.save_mutable(&mutable).await?; |
There was a problem hiding this comment.
This seems to be doing disk I/O while holding the "self.mutable" lock. I think you may want to serialize the data under the lock, but then do the actual writing to disk without that lock.
pcnofelt
left a comment
pcnofelt
left a comment
There was a problem hiding this comment.
Nothing officially blocking, gave feedback mostly on performance and order of operations.
For the notify_change() comment, it would be nice if we had a rule/best practices for how we want to do this in all cases w/r/t persistence — at time guaranteed persistence or eventual persistence and the cascading consequences
| } | ||
|
|
||
| /// Apply removals and additions as one observed message set update. | ||
| pub(crate) async fn update_messages(&self, update: MessageUpdate) -> Result<()> { |
There was a problem hiding this comment.
It looks like update_messages holds the state lock for the entire disk write, so a slow fsync could briefly stall readers like the gossip loop. Was holding the lock across the save intentional, or should we snapshot under the lock and write outside it?
| } | ||
| mutable.adhoc_membership = membership; | ||
| } | ||
| self.notify_change(); |
There was a problem hiding this comment.
We do a notify before everything is written to disk (via persist_membership() state.save()). If we want to feel fully atomic/transactional, it feels like there is a risk with calling notify_change() prior to disk write.
There was a problem hiding this comment.
I think that's ok and honestly slightly preferable. The semantics of this aren't like a database, we're not committing that the data is stored. We're just telling users of the state that there is new information they can act on. I.e. the trust engine can get started building out the new derivation without having to wait on the disk right completing.
Suppose there is a failure and the trust engine finishes its derivation before we persist to disk. Well in that case, when we reboot we will believe some marginally out of date information until we get our next gossip, which seems fine to me.
| for ip in &base.ips { | ||
| if constraint.permitted_subnets.iter().any(|s| s.contains(ip)) { | ||
| changed |= self | ||
| .imid_to_ip | ||
| .entry(target.clone()) | ||
| .or_default() | ||
| .insert(*ip); | ||
| } | ||
| } | ||
|
|
||
| for name in &base.names { | ||
| // If the constraint contains the name, or any of the | ||
| // constraint's patterns contains the name then we can | ||
| // endorse it. | ||
| if constraint | ||
| .permitted_patterns | ||
| .iter() | ||
| .any(|pattern| pattern.matches(name)) | ||
| { | ||
| changed |= self | ||
| .name_to_imid | ||
| .entry(name.clone()) | ||
| .or_default() | ||
| .insert(target.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Just calling out that this runs a lot, and switching from HashMap to BTreeMap has a performance impact we should at least measure (unless you've already done this), or I'm overlooking something here.
There was a problem hiding this comment.
yeah I think its too early to optimize this.
There's a huge optimization called semi-naive execution that @hwei0 is working on. Anything we do now isn't going to matter relative to that, and a lot of it is subject to change.
After that we can profile and see what actually matters. My personal guess is that these btree variants are actually going to be a lot faster cause they're more cache efficient, but again its kind of moot until we get the big algorithmic stuff in place.
| let (rm_endorsements, rm_revocations) = | ||
| Self::retire_endorsements(&endorsements, &revocations, now); | ||
|
|
||
| // We have to mirror the plan store updates to our local copy. This is a |
There was a problem hiding this comment.
super small, but what is meant by "plan store"
Trust engine benchmarks were still building signed endorsements and then immediately stripping them back to bases before calling TrustEngine::update. Have the harness synthesize the BTreeSet<Base> consumed by update directly so the benchmark setup matches the trust-engine API shape.
Most of Intermesh's local state lives in derivation views, state dumps, message stores, gossip peer bookkeeping, and test fixtures. These structures are built up once and then iterated, serialized, dumped, or compared. An earlier change moved message snapshots to BTreeMap and BTreeSet so their dumps would be deterministic, but the rest of the code still reached for HashMap and HashSet by default. That left iteration order tied to hash randomization, pushed callers and tests to sort around otherwise stable data, and gave up locality on a workload that rarely benefits from amortized O(1) lookup. This commit broadens the snapshot change into a repo-wide preference for ordered collections. Surfaces that are primarily iterated, serialized, dumped, compared in tests, or used as small deterministic sets now use BTreeMap and BTreeSet directly. The motivation is twofold. Ordered iteration makes behavior, dumped output, and test assertions deterministic without ad hoc sorting at use sites. And because these collections are small and dominated by full iteration rather than random lookup, BTree variants are likely faster than HashMap thanks to denser, more cache-friendly storage, and certainly more memory efficient than hash tables carrying spare capacity.
Before this change, State persisted the observed endorsement database but callers reached the messages through an exposed message::Store handle. That split made persistence and change notification a cross-module protocol: mutate the store, save State afterward, and subscribe to the right store-level signal. This commit folds the store implementation into state/messages.rs as an internal helper, and exposes message reads, updates, generation, and subscription as typed methods on State. update_messages now persists changed message sets as part of the State mutation path and bumps a single durable-state change signal that also covers ad-hoc membership.
pcnofelt
left a comment
pcnofelt
left a comment
There was a problem hiding this comment.
Took another look — lock-hold stuff is resolved (snapshot under the lock, write outside), thanks for that.
One non-blocking thing: on the notify_change() ordering, I'm fine with eventual persistence here, only wrinkle is update_messages now saves before notiyfing while persist_membership notifies first, so the two paths disagree.
Probably worth just picking the rule (sounds like notify-first?) and making them match. Filed IM-207 for it so it doesn't hold this up.
|
Fair point, though persist membership is about to be delete with the removal of adhoc anyways so I'm just going to leave it. |
3 participants
First shifts everything to the btree variants of maps/sets. There's some value in using a consistent datatype (so we don't need to convert).
Second it moves messages.rs into state. So state becomes the place where we store all of the state (messages or otherwise). This simplifies the public interfaces quite a bit, and removes some awkward manual state saving callers had to do.