From cf24fa511d61da2e5d87246aad01eccced587479 Mon Sep 17 00:00:00 2001 From: clover caruso Date: Sat, 19 Sep 2026 19:19:01 -0700 Subject: [PATCH] perf: share one GUID across new identities; verify coalesced saves in the soak harness new_id drew a fresh GUID per object, and a stored object carries an id-table entry per distinct GUID it references in every revision that rewrites it. Identities now share a GUID per 255 allocations, as OneNote numbers objects within a session, so content authored in one session costs its containers one table entry. (The restart soak that exposed the table cost starts a process per two edits and is unchanged by this: one GUID per launch either way.) The mixed-client soak verifier predated save coalescing and rejected intents shared by consecutive operations; it now accounts a coalesced group as one publication carrying every token, and still rejects a duplicated receipt. Assisted-by: claude-fable-5.1 --- crates/notebook/tests/sync_paragraph.rs | 5 ++++- crates/onestore/src/page/text.rs | 23 ++++++++++++++++------- fuzz/fuzz_targets/protected_write.rs | 4 +++- tools/native_stress.py | 2 +- tools/offline_history.py | 25 ++++++++++++++++++------- tools/test_offline_history.py | 19 +++++++++++++++++++ 6 files changed, 61 insertions(+), 17 deletions(-) diff --git a/crates/notebook/tests/sync_paragraph.rs b/crates/notebook/tests/sync_paragraph.rs index a8967ed404d913f808a78988aed7f3dc92ba877b..bf65b46c72c50cca93af4e975625c659b6f54461 100644 --- a/crates/notebook/tests/sync_paragraph.rs +++ b/crates/notebook/tests/sync_paragraph.rs @@ -70,7 +70,10 @@ fn locate_in( fn split_paragraph(page: &mut Page, text: ExGuid, offset: u32) { locate_in(page, text, |list, index| { // One identity per split, numbered as OneNote numbers the paragraph it creates. - let guid = new_id().unwrap().guid; + // A GUID of the split's own: `new_id` shares its GUID across identities. + let ExGuid { mut guid, n } = new_id().unwrap(); + guid[14] ^= n as u8; + guid[15] ^= 0xff; let mut right = list[index].clone(); right.id = ExGuid { guid, n: 1 }; right.tags.clear(); diff --git a/crates/onestore/src/page/text.rs b/crates/onestore/src/page/text.rs index 8cef50349a53ebdfd299447bf4d170fc50c0ca1a..6af7bfc8324c23542d46cdb70a4183a069f2fd17 100644 --- a/crates/onestore/src/page/text.rs +++ b/crates/onestore/src/page/text.rs @@ -153,14 +153,23 @@ impl From for crate::Error { } } -/// A fresh random identity for a new paragraph or text object. -/// A fresh identity with `n` 1: OneNote never stores an object as `{guid},0`, and OneNote -/// 2010 loses outline elements stored that way (corpus/math-edit/native-drop). +/// A fresh identity for a new paragraph or text object. Identities share one random GUID +/// per 255 allocations, as OneNote's do within a session: an object's stored id table has +/// an entry per distinct GUID it references, in every revision that rewrites it. `n` starts +/// at 1: OneNote never stores an object as `{guid},0`, and OneNote 2010 loses outline +/// elements stored that way (corpus/math-edit/native-drop). pub fn new_id() -> Result { - Ok(ExGuid { - guid: crate::write::fresh_guid().map_err(|_| EditError::Identity)?, - n: 1, - }) + static NEXT: std::sync::Mutex> = std::sync::Mutex::new(None); + let mut next = NEXT.lock().map_err(|_| EditError::Identity)?; + let id = match *next { + Some(id) => id, + None => ExGuid { + guid: crate::write::fresh_guid().map_err(|_| EditError::Identity)?, + n: 1, + }, + }; + *next = (id.n < 255).then_some(ExGuid { n: id.n + 1, ..id }); + Ok(id) } impl Paragraph { diff --git a/fuzz/fuzz_targets/protected_write.rs b/fuzz/fuzz_targets/protected_write.rs index d53ab85164e265629325de058942dbee7f27555b..d57de6d94fe4de7a167f3b2a3f33897a45d2aacf 100644 --- a/fuzz/fuzz_targets/protected_write.rs +++ b/fuzz/fuzz_targets/protected_write.rs @@ -45,7 +45,9 @@ fuzz_target!(|data: &[u8]| { let stop = (start + u32::from(step[2]) % 8).min(end); // Marked so its absence from the stored bytes is checkable. let inserted = format!("\u{1f512}sealed\u{1f512}{}", String::from_utf8_lossy(&step[3..]).replace('\0', "")); - let format = text.format_at(start.min(end.saturating_sub(1))).unwrap().clone(); + let Ok(format) = text.format_at(start.min(end.saturating_sub(1))).cloned() else { + return; + }; let edit = Edit { range: start..stop, replacement: Paragraph::new(inserted.clone(), format), diff --git a/tools/native_stress.py b/tools/native_stress.py index dd486844600f9d0035f900f166c8ceba61a18f3b..ba869c4d74b10b6655433b49ddd68644715b823c 100644 --- a/tools/native_stress.py +++ b/tools/native_stress.py @@ -219,7 +219,7 @@ def exercise(output, shared, clients, action, wait_action, wait_text, checkpoint (output / 'clocks.json').write_text(json.dumps(clocks, indent=2)) rust = {actor: [json.loads(line) for line in (folder / f'{actor}.jsonl').read_text().splitlines()] for actor in processes} commits, expected_rust = edit_history(rust, operations, edit, offline=config.get('offline', False)) - assert len(commits) == rust_writers * operations, 'Missing Rust acknowledgements' + assert sum(event.get('operations', 1) for event in commits) == rust_writers * operations, 'Missing Rust acknowledgements' native_events = [] for client in clients: local = client['folder'] / 'stress-events.jsonl' diff --git a/tools/offline_history.py b/tools/offline_history.py index 62556cc93dff529752d849a8559254da18c4eb09..794d06884b5b11f99487909c41d8b20bcfc31e33 100644 --- a/tools/offline_history.py +++ b/tools/offline_history.py @@ -20,7 +20,6 @@ def publication_links(logs, operations, partial=False): edits = [event for event in events if event['event'] == 'local_commit'] assert [event['operation'] for event in edits] == list(range(len(edits))), 'Missing or duplicate local acknowledgement' assert len(edits) <= operations and (partial or len(edits) == operations), 'Local operation count differs' - assert len({event['id'] for event in edits}) == len(edits), 'Duplicate local intent ID' assert [event['id'] for event in edits] == sorted(event['id'] for event in edits), 'Local IDs went backwards' for event in edits: token = f' [{actor}:{event["operation"]}]' @@ -37,16 +36,28 @@ def publication_links(logs, operations, partial=False): seen_revisions = set() for actor, events in logs.items(): if not actor.startswith('w'): continue - edits = {event['id']: event for event in events if event['event'] == 'local_commit'} - receipts = [event for event in events if event['event'] == 'remote_receipt'] - assert len({event['id'] for event in receipts}) == len(receipts), 'Duplicate remote receipt' + # A save replaces the newest pending save of its page, so consecutive operations + # can share one intent and one publication. + edits = {} + for event in events: + if event['event'] == 'local_commit': edits.setdefault(event['id'], []).append(event) + def once(rows): + # The client reports a coalesced intent's receipt once per operation. + found, seen = {}, {} + for row in rows: + assert found.setdefault(row['id'], row)['revision'] == row['revision'], 'One intent has two receipts' + seen[row['id']] = seen.get(row['id'], 0) + 1 + assert all(count == len(edits.get(id, [])) for id, count in seen.items()), 'Duplicate remote receipt' + return list(found.values()) + receipts = once(event for event in events if event['event'] == 'remote_receipt') assert set(event['id'] for event in receipts) <= set(edits), 'Receipt lacks a local intent' assert partial or len(receipts) == len(edits), 'Local success lacks remote acknowledgement' - reopened = [event for event in events if event['event'] == 'reopened_receipt'] + reopened = once(event for event in events if event['event'] == 'reopened_receipt') if not partial: assert [(event['id'], event['revision']) for event in reopened] == [(event['id'], event['revision']) for event in receipts], 'Receipt changed across reopen' for receipt in receipts: - intent = edits[receipt['id']] + group = edits[receipt['id']] + intent = {**group[0], 'token': ''.join(event['token'] for event in group)} attempts = [event for event in events if event['event'] == 'remote_attempt' and event['revision'] == receipt['revision']] assert len(attempts) == 1, 'Receipt does not identify one publication attempt' attempt, = attempts @@ -68,6 +79,6 @@ def publication_links(logs, operations, partial=False): assert attempt['after'] == attempt['before'] + intent['token'], 'Remote publication differs from local intent' tokens(attempt['after']) assert attempt['before'] not in links, 'Remote publications branched from the same content' - event = {**attempt, 'event': 'commit', 'operation': intent['operation'], 'token': intent['token'], 'finished_us': receipt['at_us']} + event = {**attempt, 'event': 'commit', 'operation': group[-1]['operation'], 'operations': len(group), 'token': intent['token'], 'finished_us': receipt['at_us']} links[attempt['before']] = event, attempt['after'] return links diff --git a/tools/test_offline_history.py b/tools/test_offline_history.py index eb27594af0378f16ec5e2ad3f07a190ad7ce417a..8d46ed968043acc330cfbbe8b958835d7f90ba38 100644 --- a/tools/test_offline_history.py +++ b/tools/test_offline_history.py @@ -25,6 +25,25 @@ class OfflineHistoryTests(unittest.TestCase): self.assertEqual([event['token'] for event in commits], [' [w1:0]', ' [w0:0]']) self.assertEqual(text, self.logs['r0'][1]['text']) + def test_a_save_that_replaced_a_pending_save_shares_its_intent_and_publication(self): + base = 'Concurrent edits:' + logs = {'w0': [ + {'event': 'ready', 'offline': True}, + {'event': 'local_commit', 'id': 1, 'operation': 0, 'before': base, 'token': ' [w0:0]', 'started_us': 1, 'finished_us': 2}, + {'event': 'local_commit', 'id': 1, 'operation': 1, 'before': base + ' [w0:0]', 'token': ' [w0:1]', 'started_us': 3, 'finished_us': 4}, + {'event': 'remote_attempt', 'revision': 'a', 'before': base, 'after': base + ' [w0:0] [w0:1]', 'state': 'Committed', 'started_us': 5, 'finished_us': 6}, + {'event': 'remote_receipt', 'id': 1, 'revision': 'a', 'at_us': 7}, + {'event': 'remote_receipt', 'id': 1, 'revision': 'a', 'at_us': 7}, + {'event': 'reopened_receipt', 'id': 1, 'revision': 'a'}, + {'event': 'reopened_receipt', 'id': 1, 'revision': 'a'}, + {'event': 'done'}, + ], 'r0': [{'event': 'ready'}, {'event': 'done'}]} + commits, text = edit_history(logs, 2, offline=True) + self.assertEqual([(event['token'], event['operations']) for event in commits], [(' [w0:0] [w0:1]', 2)]) + self.assertEqual(text, base + ' [w0:0] [w0:1]') + logs['w0'][3]['after'] = base + ' [w0:1]' + with self.assertRaises(AssertionError): edit_history(logs, 2, offline=True) + def test_false_receipts_lost_local_intents_and_changed_reopen_state_fail(self): for index, field, value in [(1, 'id', 2), (1, 'token', ' [w0:9]'), (3, 'state', 'Unknown'), (3, 'after', 'Concurrent edits: [w1:0]'), (4, 'at_us', 0), -- 2.54.0