From f2c73e9ee2bd094f21f55dc97c5ad8d2f3a51621 Mon Sep 17 00:00:00 2001 From: bfredl Date: Mon, 19 Feb 2024 12:00:26 +0100 Subject: [PATCH] refactor(api): reduce temporary allocations when replacing lines The way ml_replace_buf is implemented makes it unfriendly for being used in a loop: every call allocates a scratch buffer for putting the line into the "dirty" state. This then immediately needs to be freed as the next ml_replace_buf and/or ml_append_buf call will flush that buffer. It's better to later pay the price of allocating the scratch buffer only if the line is being immediately edited (likely when using the API to only change one line) with an extra memcpy, than allocating that buffer multiple times every time the API is called. Of course, a separate xmalloc/xfree cycle for each time the dirty line changes is unwanted to begin with. But fixing that is a later refactor. --- src/nvim/api/buffer.c | 67 ++++++++++++--------------------------- src/nvim/api/deprecated.c | 17 +++++----- src/nvim/api/vim.c | 8 ++--- src/nvim/ex_cmds.c | 2 +- src/nvim/fold.c | 4 +-- src/nvim/memline.c | 43 +++++++++++++++---------- src/nvim/memory.c | 10 ++++-- src/nvim/runtime.c | 2 +- 8 files changed, 73 insertions(+), 80 deletions(-) diff --git a/src/nvim/api/buffer.c b/src/nvim/api/buffer.c index ddca85945a..ad25451054 100644 --- a/src/nvim/api/buffer.c +++ b/src/nvim/api/buffer.c @@ -325,7 +325,8 @@ ArrayOf(String) nvim_buf_get_lines(uint64_t channel_id, /// @param replacement Array of lines to use as replacement /// @param[out] err Error details, if any void nvim_buf_set_lines(uint64_t channel_id, Buffer buffer, Integer start, Integer end, - Boolean strict_indexing, ArrayOf(String) replacement, Error *err) + Boolean strict_indexing, ArrayOf(String) replacement, Arena *arena, + Error *err) FUNC_API_SINCE(1) FUNC_API_TEXTLOCK_ALLOW_CMDWIN { @@ -360,14 +361,14 @@ void nvim_buf_set_lines(uint64_t channel_id, Buffer buffer, Integer start, Integ size_t new_len = replacement.size; size_t old_len = (size_t)(end - start); ptrdiff_t extra = 0; // lines added to text, can be negative - char **lines = (new_len != 0) ? xcalloc(new_len, sizeof(char *)) : NULL; + char **lines = (new_len != 0) ? arena_alloc(arena, new_len * sizeof(char *), true) : NULL; for (size_t i = 0; i < new_len; i++) { const String l = replacement.items[i].data.string; // Fill lines[i] with l's contents. Convert NULs to newlines as required by // NL-used-for-NUL. - lines[i] = xmemdupz(l.data, l.size); + lines[i] = arena_memdupz(arena, l.data, l.size); memchrsub(lines[i], NUL, NL, l.size); } @@ -412,15 +413,12 @@ void nvim_buf_set_lines(uint64_t channel_id, Buffer buffer, Integer start, Integ goto end; }); - if (ml_replace_buf(buf, (linenr_T)lnum, lines[i], false) == FAIL) { + if (ml_replace_buf(buf, (linenr_T)lnum, lines[i], false, true) == FAIL) { api_set_error(err, kErrorTypeException, "Failed to replace line"); goto end; } inserted_bytes += (bcount_t)strlen(lines[i]) + 1; - // Mark lines that haven't been passed to the buffer as they need - // to be freed later - lines[i] = NULL; } // Now we may need to insert the remaining new old_len @@ -438,9 +436,6 @@ void nvim_buf_set_lines(uint64_t channel_id, Buffer buffer, Integer start, Integ inserted_bytes += (bcount_t)strlen(lines[i]) + 1; - // Same as with replacing, but we also need to free lines - xfree(lines[i]); - lines[i] = NULL; extra++; } @@ -463,11 +458,6 @@ void nvim_buf_set_lines(uint64_t channel_id, Buffer buffer, Integer start, Integ } end: - for (size_t i = 0; i < new_len; i++) { - xfree(lines[i]); - } - - xfree(lines); try_end(err); } @@ -500,7 +490,8 @@ end: /// @param replacement Array of lines to use as replacement /// @param[out] err Error details, if any void nvim_buf_set_text(uint64_t channel_id, Buffer buffer, Integer start_row, Integer start_col, - Integer end_row, Integer end_col, ArrayOf(String) replacement, Error *err) + Integer end_row, Integer end_col, ArrayOf(String) replacement, Arena *arena, + Error *err) FUNC_API_SINCE(7) FUNC_API_TEXTLOCK_ALLOW_CMDWIN { @@ -535,33 +526,31 @@ void nvim_buf_set_text(uint64_t channel_id, Buffer buffer, Integer start_row, In return; }); - char *str_at_start = NULL; - char *str_at_end = NULL; - - // Another call to ml_get_buf() may free the line, so make a copy. - str_at_start = xstrdup(ml_get_buf(buf, (linenr_T)start_row)); + // Another call to ml_get_buf() may free the lines, so we make copies + char *str_at_start = ml_get_buf(buf, (linenr_T)start_row); size_t len_at_start = strlen(str_at_start); + str_at_start = arena_memdupz(arena, str_at_start, len_at_start); start_col = start_col < 0 ? (int64_t)len_at_start + start_col + 1 : start_col; VALIDATE_RANGE((start_col >= 0 && (size_t)start_col <= len_at_start), "start_col", { - goto early_end; + return; }); - // Another call to ml_get_buf() may free the line, so make a copy. - str_at_end = xstrdup(ml_get_buf(buf, (linenr_T)end_row)); + char *str_at_end = ml_get_buf(buf, (linenr_T)end_row); size_t len_at_end = strlen(str_at_end); + str_at_end = arena_memdupz(arena, str_at_end, len_at_end); end_col = end_col < 0 ? (int64_t)len_at_end + end_col + 1 : end_col; VALIDATE_RANGE((end_col >= 0 && (size_t)end_col <= len_at_end), "end_col", { - goto early_end; + return; }); VALIDATE((start_row <= end_row && !(end_row == start_row && start_col > end_col)), "%s", "'start' is higher than 'end'", { - goto early_end; + return; }); bool disallow_nl = (channel_id != VIML_INTERNAL_CALL); if (!check_string_array(replacement, "replacement string", disallow_nl, err)) { - goto early_end; + return; } size_t new_len = replacement.size; @@ -591,7 +580,7 @@ void nvim_buf_set_text(uint64_t channel_id, Buffer buffer, Integer start_row, In if (replacement.size == 1) { firstlen += last_part_len; } - char *first = xmallocz(firstlen); + char *first = arena_allocz(arena, firstlen); char *last = NULL; memcpy(first, str_at_start, (size_t)start_col); memcpy(first + start_col, first_item.data, first_item.size); @@ -599,13 +588,13 @@ void nvim_buf_set_text(uint64_t channel_id, Buffer buffer, Integer start_row, In if (replacement.size == 1) { memcpy(first + start_col + first_item.size, str_at_end + end_col, last_part_len); } else { - last = xmallocz(last_item.size + last_part_len); + last = arena_allocz(arena, last_item.size + last_part_len); memcpy(last, last_item.data, last_item.size); memchrsub(last, NUL, NL, last_item.size); memcpy(last + last_item.size, str_at_end + end_col, last_part_len); } - char **lines = xcalloc(new_len, sizeof(char *)); + char **lines = arena_alloc(arena, new_len * sizeof(char *), true); lines[0] = first; new_byte += (bcount_t)(first_item.size); for (size_t i = 1; i < new_len - 1; i++) { @@ -613,7 +602,7 @@ void nvim_buf_set_text(uint64_t channel_id, Buffer buffer, Integer start_row, In // Fill lines[i] with l's contents. Convert NULs to newlines as required by // NL-used-for-NUL. - lines[i] = xmemdupz(l.data, l.size); + lines[i] = arena_memdupz(arena, l.data, l.size); memchrsub(lines[i], NUL, NL, l.size); new_byte += (bcount_t)(l.size) + 1; } @@ -665,13 +654,10 @@ void nvim_buf_set_text(uint64_t channel_id, Buffer buffer, Integer start_row, In goto end; }); - if (ml_replace_buf(buf, (linenr_T)lnum, lines[i], false) == FAIL) { + if (ml_replace_buf(buf, (linenr_T)lnum, lines[i], false, true) == FAIL) { api_set_error(err, kErrorTypeException, "Failed to replace line"); goto end; } - // Mark lines that haven't been passed to the buffer as they need - // to be freed later - lines[i] = NULL; } // Now we may need to insert the remaining new old_len @@ -687,9 +673,6 @@ void nvim_buf_set_text(uint64_t channel_id, Buffer buffer, Integer start_row, In goto end; } - // Same as with replacing, but we also need to free lines - xfree(lines[i]); - lines[i] = NULL; extra++; } @@ -722,15 +705,7 @@ void nvim_buf_set_text(uint64_t channel_id, Buffer buffer, Integer start_row, In } end: - for (size_t i = 0; i < new_len; i++) { - xfree(lines[i]); - } - xfree(lines); try_end(err); - -early_end: - xfree(str_at_start); - xfree(str_at_end); } /// Gets a range from the buffer. diff --git a/src/nvim/api/deprecated.c b/src/nvim/api/deprecated.c index 9b8cacd7c2..f805c945f3 100644 --- a/src/nvim/api/deprecated.c +++ b/src/nvim/api/deprecated.c @@ -225,12 +225,12 @@ Dictionary nvim_get_hl_by_name(String name, Boolean rgb, Arena *arena, Error *er /// the end of the buffer. /// @param lines Array of lines /// @param[out] err Error details, if any -void buffer_insert(Buffer buffer, Integer lnum, ArrayOf(String) lines, Error *err) +void buffer_insert(Buffer buffer, Integer lnum, ArrayOf(String) lines, Arena *arena, Error *err) FUNC_API_DEPRECATED_SINCE(1) { // "lnum" will be the index of the line after inserting, // no matter if it is negative or not - nvim_buf_set_lines(0, buffer, lnum, lnum, true, lines, err); + nvim_buf_set_lines(0, buffer, lnum, lnum, true, lines, arena, err); } /// Gets a buffer line @@ -272,13 +272,13 @@ String buffer_get_line(Buffer buffer, Integer index, Arena *arena, Error *err) /// @param index Line index /// @param line Contents of the new line /// @param[out] err Error details, if any -void buffer_set_line(Buffer buffer, Integer index, String line, Error *err) +void buffer_set_line(Buffer buffer, Integer index, String line, Arena *arena, Error *err) FUNC_API_DEPRECATED_SINCE(1) { Object l = STRING_OBJ(line); Array array = { .items = &l, .size = 1 }; index = convert_index(index); - nvim_buf_set_lines(0, buffer, index, index + 1, true, array, err); + nvim_buf_set_lines(0, buffer, index, index + 1, true, array, arena, err); } /// Deletes a buffer line @@ -291,12 +291,12 @@ void buffer_set_line(Buffer buffer, Integer index, String line, Error *err) /// @param buffer buffer handle /// @param index line index /// @param[out] err Error details, if any -void buffer_del_line(Buffer buffer, Integer index, Error *err) +void buffer_del_line(Buffer buffer, Integer index, Arena *arena, Error *err) FUNC_API_DEPRECATED_SINCE(1) { Array array = ARRAY_DICT_INIT; index = convert_index(index); - nvim_buf_set_lines(0, buffer, index, index + 1, true, array, err); + nvim_buf_set_lines(0, buffer, index, index + 1, true, array, arena, err); } /// Retrieves a line range from the buffer @@ -342,12 +342,13 @@ ArrayOf(String) buffer_get_line_slice(Buffer buffer, // array will delete the line range) /// @param[out] err Error details, if any void buffer_set_line_slice(Buffer buffer, Integer start, Integer end, Boolean include_start, - Boolean include_end, ArrayOf(String) replacement, Error *err) + Boolean include_end, ArrayOf(String) replacement, Arena *arena, + Error *err) FUNC_API_DEPRECATED_SINCE(1) { start = convert_index(start) + !include_start; end = convert_index(end) + include_end; - nvim_buf_set_lines(0, buffer, start, end, false, replacement, err); + nvim_buf_set_lines(0, buffer, start, end, false, replacement, arena, err); } /// Sets a buffer-scoped (b:) variable diff --git a/src/nvim/api/vim.c b/src/nvim/api/vim.c index e472f5d160..c6043a2871 100644 --- a/src/nvim/api/vim.c +++ b/src/nvim/api/vim.c @@ -675,21 +675,21 @@ String nvim_get_current_line(Arena *arena, Error *err) /// /// @param line Line contents /// @param[out] err Error details, if any -void nvim_set_current_line(String line, Error *err) +void nvim_set_current_line(String line, Arena *arena, Error *err) FUNC_API_SINCE(1) FUNC_API_TEXTLOCK_ALLOW_CMDWIN { - buffer_set_line(curbuf->handle, curwin->w_cursor.lnum - 1, line, err); + buffer_set_line(curbuf->handle, curwin->w_cursor.lnum - 1, line, arena, err); } /// Deletes the current line. /// /// @param[out] err Error details, if any -void nvim_del_current_line(Error *err) +void nvim_del_current_line(Arena *arena, Error *err) FUNC_API_SINCE(1) FUNC_API_TEXTLOCK_ALLOW_CMDWIN { - buffer_del_line(curbuf->handle, curwin->w_cursor.lnum - 1, err); + buffer_del_line(curbuf->handle, curwin->w_cursor.lnum - 1, arena, err); } /// Gets a global (g:) variable. diff --git a/src/nvim/ex_cmds.c b/src/nvim/ex_cmds.c index 70146a8602..74ad8e95a2 100644 --- a/src/nvim/ex_cmds.c +++ b/src/nvim/ex_cmds.c @@ -4674,7 +4674,7 @@ static int show_sub(exarg_T *eap, pos_T old_cusr, PreviewLines *preview_lines, i snprintf(str, line_size, "|%*" PRIdLINENR "| %s", col_width - 3, next_linenr, line); if (linenr_preview == 0) { - ml_replace_buf(cmdpreview_buf, 1, str, true); + ml_replace_buf(cmdpreview_buf, 1, str, true, false); } else { ml_append_buf(cmdpreview_buf, linenr_preview, str, (colnr_T)line_size, false); } diff --git a/src/nvim/fold.c b/src/nvim/fold.c index 0ef9e48e3e..c571aaf0a4 100644 --- a/src/nvim/fold.c +++ b/src/nvim/fold.c @@ -1626,7 +1626,7 @@ static void foldAddMarker(buf_T *buf, pos_T pos, const char *marker, size_t mark STRCPY(newline + line_len + (p - cms) + markerlen, p + 2); added = markerlen + strlen(cms) - 2; } - ml_replace_buf(buf, lnum, newline, false); + ml_replace_buf(buf, lnum, newline, false, false); if (added) { extmark_splice_cols(buf, (int)lnum - 1, (int)line_len, 0, (int)added, kExtmarkUndo); @@ -1690,7 +1690,7 @@ static void foldDelMarker(buf_T *buf, linenr_T lnum, char *marker, size_t marker assert(p >= line); memcpy(newline, line, (size_t)(p - line)); STRCPY(newline + (p - line), p + len); - ml_replace_buf(buf, lnum, newline, false); + ml_replace_buf(buf, lnum, newline, false, false); extmark_splice_cols(buf, (int)lnum - 1, (int)(p - line), (int)len, 0, kExtmarkUndo); } diff --git a/src/nvim/memline.c b/src/nvim/memline.c index e463f66714..6b2f26b2d8 100644 --- a/src/nvim/memline.c +++ b/src/nvim/memline.c @@ -1694,7 +1694,7 @@ void ml_sync_all(int check_file, int check_char, bool do_fsync) if (buf->b_ml.ml_mfp == NULL || buf->b_ml.ml_mfp->mf_fname == NULL) { continue; // no file } - ml_flush_line(buf); // flush buffered line + ml_flush_line(buf, false); // flush buffered line // flush locked block ml_find_line(buf, 0, ML_FLUSH); if (bufIsChanged(buf) && check_file && mf_need_trans(buf->b_ml.ml_mfp) @@ -1745,7 +1745,7 @@ void ml_preserve(buf_T *buf, bool message, bool do_fsync) // before. got_int = false; - ml_flush_line(buf); // flush buffered line + ml_flush_line(buf, false); // flush buffered line ml_find_line(buf, 0, ML_FLUSH); // flush locked block int status = mf_sync(mfp, MFS_ALL | (do_fsync ? MFS_FLUSH : 0)); @@ -1862,7 +1862,7 @@ static char *ml_get_buf_impl(buf_T *buf, linenr_T lnum, bool will_change) siemsg(_(e_ml_get_invalid_lnum_nr), (int64_t)lnum); recursive--; } - ml_flush_line(buf); + ml_flush_line(buf, false); errorret: STRCPY(questions, "???"); buf->b_ml.ml_line_lnum = lnum; @@ -1881,7 +1881,7 @@ errorret: // Don't use the last used line when 'swapfile' is reset, need to load all // blocks. if (buf->b_ml.ml_line_lnum != lnum) { - ml_flush_line(buf); + ml_flush_line(buf, false); // Find the data block containing the line. // This also fills the stack with the blocks from the root to the data @@ -1964,7 +1964,7 @@ int ml_append(linenr_T lnum, char *line, colnr_T len, bool newfile) } if (curbuf->b_ml.ml_line_lnum != 0) { - ml_flush_line(curbuf); + ml_flush_line(curbuf, false); } return ml_append_int(curbuf, lnum, line, len, newfile, false); } @@ -1984,7 +1984,7 @@ int ml_append_buf(buf_T *buf, linenr_T lnum, char *line, colnr_T len, bool newfi } if (buf->b_ml.ml_line_lnum != 0) { - ml_flush_line(buf); + ml_flush_line(buf, false); } return ml_append_int(buf, lnum, line, len, newfile, false); } @@ -2423,7 +2423,7 @@ void ml_add_deleted_len_buf(buf_T *buf, char *ptr, ssize_t len) int ml_replace(linenr_T lnum, char *line, bool copy) { - return ml_replace_buf(curbuf, lnum, line, copy); + return ml_replace_buf(curbuf, lnum, line, copy, false); } /// Replace line "lnum", with buffering, in current buffer. @@ -2438,7 +2438,7 @@ int ml_replace(linenr_T lnum, char *line, bool copy) /// changed_lines(), unless update_screen(UPD_NOT_VALID) is used. /// /// @return FAIL for failure, OK otherwise -int ml_replace_buf(buf_T *buf, linenr_T lnum, char *line, bool copy) +int ml_replace_buf(buf_T *buf, linenr_T lnum, char *line, bool copy, bool noalloc) { if (line == NULL) { // just checking... return FAIL; @@ -2450,12 +2450,13 @@ int ml_replace_buf(buf_T *buf, linenr_T lnum, char *line, bool copy) } if (copy) { + assert(!noalloc); line = xstrdup(line); } if (buf->b_ml.ml_line_lnum != lnum) { // another line is buffered, flush it - ml_flush_line(buf); + ml_flush_line(buf, false); } if (kv_size(buf->update_callbacks)) { @@ -2469,6 +2470,13 @@ int ml_replace_buf(buf_T *buf, linenr_T lnum, char *line, bool copy) buf->b_ml.ml_line_ptr = line; buf->b_ml.ml_line_lnum = lnum; buf->b_ml.ml_flags = (buf->b_ml.ml_flags | ML_LINE_DIRTY) & ~ML_EMPTY; + if (noalloc) { + // TODO(bfredl): this is a bit of a hack. but replacing lines in a loop is really common, + // and allocating a separate scratch buffer for each line which is immediately freed adds + // a lot of noise. A more general refactor could be to use a _fixed_ scratch buffer for + // all lines up to $REASONABLE_SIZE . + ml_flush_line(buf, true); + } return OK; } @@ -2483,7 +2491,7 @@ int ml_replace_buf(buf_T *buf, linenr_T lnum, char *line, bool copy) /// @return FAIL for failure, OK otherwise int ml_delete(linenr_T lnum, bool message) { - ml_flush_line(curbuf); + ml_flush_line(curbuf, false); return ml_delete_int(curbuf, lnum, message); } @@ -2496,7 +2504,7 @@ int ml_delete(linenr_T lnum, bool message) /// @return FAIL for failure, OK otherwise int ml_delete_buf(buf_T *buf, linenr_T lnum, bool message) { - ml_flush_line(buf); + ml_flush_line(buf, false); return ml_delete_int(buf, lnum, message); } @@ -2516,7 +2524,7 @@ static int ml_delete_int(buf_T *buf, linenr_T lnum, bool message) set_keep_msg(_(no_lines_msg), 0); } - int i = ml_replace_buf(buf, 1, "", true); + int i = ml_replace_buf(buf, 1, "", true, false); buf->b_ml.ml_flags |= ML_EMPTY; return i; @@ -2724,7 +2732,7 @@ size_t ml_flush_deleted_bytes(buf_T *buf, size_t *codepoints, size_t *codeunits) } /// flush ml_line if necessary -static void ml_flush_line(buf_T *buf) +static void ml_flush_line(buf_T *buf, bool noalloc) { static bool entered = false; @@ -2798,10 +2806,13 @@ static void ml_flush_line(buf_T *buf) ml_delete_int(buf, lnum, false); } } - xfree(new_line); + if (!noalloc) { + xfree(new_line); + } entered = false; } else if (buf->b_ml.ml_flags & ML_ALLOCATED) { + assert(!noalloc); // caller must set ML_LINE_DIRTY with noalloc, handled above xfree(buf->b_ml.ml_line_ptr); } @@ -3903,7 +3914,7 @@ int ml_find_line_or_offset(buf_T *buf, linenr_T lnum, int *offp, bool no_ff) // was never cached to start with anyway). bool can_cache = (lnum != 0 && !ffdos && buf->b_ml.ml_line_lnum == lnum); if (lnum == 0 || buf->b_ml.ml_line_lnum < lnum || !no_ff) { - ml_flush_line(curbuf); + ml_flush_line(curbuf, false); } else if (can_cache && buf->b_ml.ml_line_offset > 0) { return (int)buf->b_ml.ml_line_offset; } @@ -4030,7 +4041,7 @@ void goto_byte(int cnt) { int boff = cnt; - ml_flush_line(curbuf); // cached line may be dirty + ml_flush_line(curbuf, false); // cached line may be dirty setpcmark(); if (boff) { boff--; diff --git a/src/nvim/memory.c b/src/nvim/memory.c index d001685e63..4520635137 100644 --- a/src/nvim/memory.c +++ b/src/nvim/memory.c @@ -667,14 +667,20 @@ void arena_mem_free(ArenaMem mem) } } -char *arena_memdupz(Arena *arena, const char *buf, size_t size) +char *arena_allocz(Arena *arena, size_t size) { char *mem = arena_alloc(arena, size + 1, false); - memcpy(mem, buf, size); mem[size] = NUL; return mem; } +char *arena_memdupz(Arena *arena, const char *buf, size_t size) +{ + char *mem = arena_allocz(arena, size); + memcpy(mem, buf, size); + return mem; +} + #if defined(EXITFREE) # include "nvim/autocmd.h" diff --git a/src/nvim/runtime.c b/src/nvim/runtime.c index fb6397f9eb..76188499e3 100644 --- a/src/nvim/runtime.c +++ b/src/nvim/runtime.c @@ -581,7 +581,7 @@ Array runtime_inspect(Arena *arena) for (size_t i = 0; i < kv_size(path); i++) { SearchPathItem *item = &kv_A(path, i); - Array entry = ARRAY_DICT_INIT; + Array entry = arena_array(arena, 3); ADD_C(entry, CSTR_AS_OBJ(item->path)); ADD_C(entry, BOOLEAN_OBJ(item->after)); if (item->has_lua != kNone) {