From a0dc3f00675f738c6e69b71aa7c6e25a99bc4b6f Mon Sep 17 00:00:00 2001 From: zeertzjq Date: Wed, 12 Aug 2026 10:00:09 +0800 Subject: [PATCH] vim-patch:9.2.0935: reading an undo file is slow with many undo headers (#41285) Problem: Reading an undo file resolves every stored sequence number with a linear scan over all headers, making loading quadratic in the number of undo states. Solution: Sort uhp_table on uh_seq once and resolve each reference with a binary search; the duplicate uh_seq check becomes a single pass over the sorted table (Samuel Schlesinger). At the default 'undolevels' of 1000 the quadratic cost is not measurable; it takes 'undolevels' in the tens of thousands to matter. Loading an undo file with 20000 states and 50 alternate branches with :rundo goes from 1.49s to 0.11s (min of 3, macOS arm64), with the same undotree(). Also make old_idx/new_idx/cur_idx and the loop index "i" long instead of short/int: they index uhp_table, whose length num_head is a long read from the file. A short index truncated above 32767 headers, making the restored b_u_oldhead/b_u_newhead/b_u_curhead pointers wrong in exactly the many-headers case this change is about. Add tests: a round-trip test with alternate branches that compares the entries of the tree and the text at every sequence number, a corruption test with a duplicated uh_seq, and a test for reading an undo file with zero headers, which is written when only the line for the "U" command is saved. closes: vim/vim#20942 https://github.com/vim/vim/commit/fccf613c8f5b550797c08a45a768e14adefd882f Co-authored-by: Samuel Schlesinger Co-authored-by: Claude --- src/nvim/undo.c | 143 ++++++++++++++++++--------------- test/old/testdir/test_undo.vim | 139 ++++++++++++++++++++++++++++++++ 2 files changed, 216 insertions(+), 66 deletions(-) diff --git a/src/nvim/undo.c b/src/nvim/undo.c index cd400a35f8..8c664b74eb 100644 --- a/src/nvim/undo.c +++ b/src/nvim/undo.c @@ -773,6 +773,7 @@ static void u_free_uhp(u_header_T *uhp) u_freeentry(uep, uep->ue_size); uep = nuep; } + kv_destroy(uhp->uh_extmark); xfree(uhp); } @@ -1365,6 +1366,38 @@ theend: } } +/// Compare undo headers on the sequence number, for sorting uhp_table in +/// u_read_undo(). +static int uhp_seq_cmp(const void *v1, const void *v2) +{ + const u_header_T *u1 = *(u_header_T **)v1; + const u_header_T *u2 = *(u_header_T **)v2; + + return u1->uh_seq == u2->uh_seq ? 0 : u1->uh_seq > u2->uh_seq ? 1 : -1; +} + +/// Find the header with sequence number "seq" in "uhp_table", which has +/// "num_head" entries and is sorted on uh_seq. +/// Return the table index of the header or -1 when not found. +static int uhp_table_find(u_header_T **uhp_table, int num_head, int seq) +{ + int lo = 0; + int hi = num_head - 1; + + while (lo <= hi) { + int mid = lo + (hi - lo) / 2; + + if (uhp_table[mid]->uh_seq < seq) { + lo = mid + 1; + } else if (uhp_table[mid]->uh_seq > seq) { + hi = mid - 1; + } else { + return mid; + } + } + return -1; +} + /// Loads the undo tree from an undo file. /// If "name" is not NULL use it as the undo file name. This also means being /// a bit more verbose. @@ -1549,84 +1582,62 @@ void u_read_undo(char *name, const uint8_t *hash, const char *orig_name FUNC_ATT # define SET_FLAG(j) #endif - // We have put all of the headers into a table. Now we iterate through the - // table and swizzle each sequence number we have stored in uh_*_seq into - // a pointer corresponding to the header with that sequence number. - int16_t old_idx = -1; - int16_t new_idx = -1; - int16_t cur_idx = -1; + // We have put all of the headers into a table. Each header stores the + // sequence numbers of the headers it links to; resolve those into + // pointers. Sort the table on uh_seq once, so that every lookup is a + // binary search instead of a linear scan, which would be quadratic + // overall. Every entry is non-NULL: a header that failed to + // unserialize or a count mismatch was an error above. + if (num_head > 0) { + qsort(uhp_table, (size_t)num_head, sizeof(u_header_T *), uhp_seq_cmp); + } + + // In the sorted table two headers with the same uh_seq are neighbours. + for (int i = 0; i < num_head - 1; i++) { + if (uhp_table[i]->uh_seq == uhp_table[i + 1]->uh_seq) { + corruption_error("duplicate uh_seq", file_name); + goto error; + } + } + + // Resolve the sequence number "link".seq into a pointer to the header + // with that number. A number that does not match any header, including + // zero (written for a NULL pointer) and the own sequence number of the + // header "hidx", resolves to NULL. +#define SWIZZLE_SEQ(link, hidx) \ + do { \ + int fidx = uhp_table_find(uhp_table, num_head, (link).seq); \ + if (fidx >= 0 && fidx != (hidx)) { \ + (link).ptr = uhp_table[fidx]; \ + SET_FLAG(fidx); \ + } else { \ + (link).ptr = NULL; \ + } \ + } while (0) + + int old_idx = -1; + int new_idx = -1; + int cur_idx = -1; for (int i = 0; i < num_head; i++) { u_header_T *uhp = uhp_table[i]; - if (uhp == NULL) { - continue; - } - for (int j = 0; j < num_head; j++) { - if (uhp_table[j] != NULL && i != j - && uhp_table[i]->uh_seq == uhp_table[j]->uh_seq) { - corruption_error("duplicate uh_seq", file_name); - goto error; - } - } - { - const int seq = uhp->uh_next.seq; - uhp->uh_next.ptr = NULL; - for (int j = 0; j < num_head; j++) { - if (uhp_table[j] != NULL && i != j && uhp_table[j]->uh_seq == seq) { - uhp->uh_next.ptr = uhp_table[j]; - SET_FLAG(j); - break; - } - } - } - { - const int seq = uhp->uh_prev.seq; - uhp->uh_prev.ptr = NULL; - for (int j = 0; j < num_head; j++) { - if (uhp_table[j] != NULL && i != j && uhp_table[j]->uh_seq == seq) { - uhp->uh_prev.ptr = uhp_table[j]; - SET_FLAG(j); - break; - } - } - } - { - const int seq = uhp->uh_alt_next.seq; - uhp->uh_alt_next.ptr = NULL; - for (int j = 0; j < num_head; j++) { - if (uhp_table[j] != NULL && i != j && uhp_table[j]->uh_seq == seq) { - uhp->uh_alt_next.ptr = uhp_table[j]; - SET_FLAG(j); - break; - } - } - } - { - const int seq = uhp->uh_alt_prev.seq; - uhp->uh_alt_prev.ptr = NULL; - for (int j = 0; j < num_head; j++) { - if (uhp_table[j] != NULL && i != j && uhp_table[j]->uh_seq == seq) { - uhp->uh_alt_prev.ptr = uhp_table[j]; - SET_FLAG(j); - break; - } - } - } + SWIZZLE_SEQ(uhp->uh_next, i); + SWIZZLE_SEQ(uhp->uh_prev, i); + SWIZZLE_SEQ(uhp->uh_alt_next, i); + SWIZZLE_SEQ(uhp->uh_alt_prev, i); if (old_header_seq > 0 && old_idx < 0 && uhp->uh_seq == old_header_seq) { - assert(i <= INT16_MAX); - old_idx = (int16_t)i; + old_idx = i; SET_FLAG(i); } if (new_header_seq > 0 && new_idx < 0 && uhp->uh_seq == new_header_seq) { - assert(i <= INT16_MAX); - new_idx = (int16_t)i; + new_idx = i; SET_FLAG(i); } if (cur_header_seq > 0 && cur_idx < 0 && uhp->uh_seq == cur_header_seq) { - assert(i <= INT16_MAX); - cur_idx = (int16_t)i; + cur_idx = i; SET_FLAG(i); } } +#undef SWIZZLE_SEQ // Now that we have read the undo info successfully, free the current undo // info and use the info from the file. diff --git a/test/old/testdir/test_undo.vim b/test/old/testdir/test_undo.vim index 8f686c594c..5705ac2afe 100644 --- a/test/old/testdir/test_undo.vim +++ b/test/old/testdir/test_undo.vim @@ -1005,4 +1005,143 @@ func Test_corrupted_undofile() let &undofile = _uf endfunc +" Test that an undo file with alternate branches round-trips: the tree +" structure and the text at every sequence number survive :wundo + :rundo. +func Test_undofile_branches() + CheckFeature persistent_undo + let save_ul = &undolevels + defer execute('let &undolevels = ' .. save_ul) + new Xubranches.txt + setl noswapfile + set ul=100 + call setline(1, 'a') + for i in range(5) + let &undolevels = &undolevels + call setline(1, 'main' .. i) + endfor + " create two alternate branches + silent undo 3 + let &undolevels = &undolevels + call setline(1, 'branch-a') + silent undo 2 + let &undolevels = &undolevels + call setline(1, 'branch-b') + + write + defer delete('Xubranches.txt') + wundo! Xubranches.undo + defer delete('Xubranches.undo') + let tree_before = undotree() + " remember the text at every undo state + let texts = {} + for seq in range(1, tree_before.seq_last) + exe 'silent undo ' .. seq + let texts[seq] = getline(1) + endfor + bwipe! + + edit Xubranches.txt + setl noswapfile + rundo Xubranches.undo + let tree_after = undotree() + call assert_equal(tree_before.seq_last, tree_after.seq_last) + " every field of the entries, including times and save numbers, must + " round-trip through the undo file exactly + call assert_equal(tree_before.entries, tree_after.entries) + for [seq, text] in items(texts) + exe 'silent undo ' .. seq + call assert_equal(text, getline(1), 'text at undo state ' .. seq) + endfor + + bwipe! +endfunc + +" Test that a duplicated sequence number in an undo file is detected. +func Test_undofile_duplicate_seq() + CheckFeature persistent_undo + let save_ul = &undolevels + defer execute('let &undolevels = ' .. save_ul) + new Xudupseq.txt + setl noswapfile + set ul=100 + call setline(1, 'one') + let &undolevels = &undolevels + call setline(1, 'two') + let &undolevels = &undolevels + call setline(1, 'three') + write + defer delete('Xudupseq.txt') + wundo! Xudupseq.undo + defer delete('Xudupseq.undo') + + " Overwrite the uh_seq of the second header with that of the first. A + " header starts with the magic bytes 0x5f 0xd0, followed by four 4-byte + " header references and then the 4-byte uh_seq. Require the references + " and uh_seq to be small numbers, so that a timestamp that happens to + " contain the magic bytes is not mistaken for a header. + let blob = readfile('Xudupseq.undo', 'B') + let headers = [] + for i in range(len(blob) - 22) + if blob[i] == 0x5f && blob[i + 1] == 0xd0 + let ok = v:true + for field in range(5) + let off = i + 2 + field * 4 + if blob[off] != 0 || blob[off + 1] != 0 || blob[off + 2] != 0 + \ || blob[off + 3] > 8 + let ok = v:false + break + endif + endfor + if ok + call add(headers, i) + endif + endif + endfor + call assert_true(len(headers) >= 2, 'found undo file headers') + let first = headers[0] + 18 + let second = headers[1] + 18 + " Check that the detected headers are the intended ones before patching + " any bytes: the headers are written oldest first, so the first two carry + " sequence numbers 1 and 2. + call assert_equal(0z00000001, blob[first : first + 3]) + call assert_equal(0z00000002, blob[second : second + 3]) + let blob[second : second + 3] = blob[first : first + 3] + call writefile(blob, 'Xudupseq.undo') + call assert_fails('rundo Xudupseq.undo', 'E825:') + + bwipe! +endfunc + +" Test reading an undo file with zero undo headers, which is written when +" only the line for the "U" command is saved, e.g. with 'undolevels' -1. +func Test_undofile_zero_headers() + CheckFeature persistent_undo + let save_ul = &undolevels + defer execute('let &undolevels = ' .. save_ul) + " The buffer must not collect any undo header, so create the file on disk + " directly and only change the buffer with 'undolevels' already negative. + call writefile(['hello', 'world'], 'Xuzero.txt', 'D') + set undolevels=-1 + edit Xuzero.txt + normal! x + wundo! Xuzero.undo + defer delete('Xuzero.undo') + bwipe! + + " Make the same change so that the buffer text matches the hash stored in + " the undo file, then read the undo file back. + edit Xuzero.txt + normal! x + let v:errmsg = '' + " a silent rejection of the undo file gives a warning, not an error + let v:warningmsg = '' + rundo Xuzero.undo + call assert_equal('', v:errmsg) + call assert_equal('', v:warningmsg) + normal! U + call assert_equal('hello', getline(1)) + + bwipe! +endfunc + " vim: shiftwidth=2 sts=2 expandtab