[RDY] Data structure improvements

[tarruda]

Some changes taken from a private branch where I refactor most libuv code to run in a background IO thread(ref #2371). Pushing as a separate PR to simplify reviewing.

[ZyX-I]

About rbuffer: can you consider implementing it on top of ringbuf.h from shada branch? I have a generic (though currently untested) ring buffer implementation there that uses macros and it is currently used for holding an array of structures (so I can’t reuse yours which holds pointers).

Though it looks like I am missing something regarding what is a ring buffer: (size_t)(buf->end_ptr - buf->start_ptr) cannot possibly be ring buffer size because it is OK for ring buffer to have start after the end. E.g. from wiki:

This image shows a full buffer with two elements having been overwritten:

(end - start) here will return -1.

[tarruda]

About rbuffer: can you consider implementing it on top of ringbuf.h from shada branch? I have a generic (though currently untested) ring buffer implementation there that uses macros and it is currently used for holding an array of structures (so I can’t reuse yours which holds pointers).

I'm gonna give it a shot

[tarruda]

Though it looks like I am missing something regarding what is a ring buffer: (size_t)(buf->end_ptr - buf->start_ptr) cannot possibly be ring buffer size because it is OK for ring buffer to have start after the end. E.g. from wiki:

In this implementation, end_ptr is a pointer to the end of the array. The last element is represented by write_ptr("end" in your diagram)

[ZyX-I]

@tarruda I have just pushed some fixes to my implementation. Manual test run OK after that.

[tarruda]

@ZyX-I I'd like to keep the possibility of passing the ring buffer pointer to functions that write or read directly to memory locations, which is why I have implemented the RBUFFER_WHILE_NOT_{FULL,EMPTY} macros.

If the library I'm interfacing with exposes it's own internal buffer, I can just use rbuffer_read/rbuffer_write which is the case for msgpack(see channel.c/parse_msgpack)

But if the library expects a pointer(libvterm/vterm_input_write for example), I'd have to copy the memory to a temporary buffer and then pass it to the library which is a bit inefficient. With terminal_receive/vterm_input_write I usee RBUFFER_WHILE_NOT_EMPTY, which will feed the data by passing the pointer directly(possibly in 2 passes since it may be necessary to wrap around the buffer).

Could you implement something similar to RBUFFER_WHILE_NOT_{FULL,EMPTY} on your generic ring buffer?

[ZyX-I]

@tarruda I guess here are the differences (yours → mine):

rbuffer_capacity() → None (using rb->buf_end - rb->buf directly when needed).
rbuffer_space() → None
rb->length → rb_length()
rbuffer_free() → rb_dealloc()
rbuffer_write_ptr(), rbuffer_read_ptr() → None (not generic)
rbuffer_produced() → None (I assume that pushing to ringbuffer is done only via rb_push or rb_insert)
rbuffer_consumed() → None (it is not assumed that reading consumes ring buffer)
rbuffer_read() → None
rbuffer_write() → a sequence of rb_push() calls
None → rb_free() (simple characters need not be freed)
None → rb_insert() (you do not need to insert into the middle of the ring buffer)
None → RINGBUF_ITER_BACK (you do not need to process ring buffer from the end to the start)
None → rb_remove() (you do not need to remove random elements)

Very different ring buffer implementations for different purposes.

[tarruda]

Just found an interesting ring buffer implementation: https://github.com/michaeltyson/TPCircularBuffer

This ones uses a virtual memory technique that avoids the need for wrapping logic, I wonder if we can do something similar without introducing compatibility issues?

[ZyX-I]

With that big amount of differences it will be easier to keep your buffer implementation, but rename it to “string ring buffer” (because mine is generic). I can add callbacks, but your variant is better suited for your purposes.

[tarruda]

With that big amount of differences it will be easier to keep your buffer implementation, but rename it to “string ring buffer” (because mine is generic). I can add callbacks, but your variant is better suited for your purposes.

👍

But later I might spend some time refactoring both into a single generic data structure. If you think about it, rb_insert/rb_remove are simply special cases of writing/reading from the buffer(could be implemented on top of rbuffer_{read,write} passing 1 as the size).

[ZyX-I]

@tarruda Nope. My rb_insert/rb_remove support inserting at/removing from any position.

By the way, what is the best variant for writing tests for it? For “manual” tests I used a standalone C file with a single nvim include (ringbuf.h). Main ideas that come to mind: use this file and base test on io.open and (I guess, better): create a helper C file which will have wrapper for all tested functions (they are static inline) and use regular unit testing infrastructure, but where this is to be located?

[oni-link]

Early return with

if (src_size == copy_count) {
   return size;
}

?

[tarruda]

👍

[tarruda]

By the way, what is the best variant for writing tests for it? For “manual” tests I used a standalone C file with a single nvim include (ringbuf.h). Main ideas that come to mind: use this file and base test on io.open and (I guess, better): create a helper C file which will have wrapper for all tested functions (they are static inline) and use regular unit testing infrastructure, but where this is to be located?

I've added a basic infrastructure for writing helpers that are only compiled in the unit tests. Basically any C file you add to test/unit/helpers will be built into the test library, so you can use it to add wrappers for iinline functions

[oni-link]

Using clang-format on the whole file eval.c (:'<,'>pyf ~/git/llvm/tools/clang/tools/clang-format/clang-format.py) gives the following error:

Error detected while processing function provider#python#Call:
line   12:
Channel was closed by the client

No error on master.

[oni-link]

If input_buffer is not read for a while and is (nearly) full, we could loose some bytes.

[tarruda]

This bug is already present(#1813) right? I intend to fix before #2371, which will be done by allocating memory for new input.

[jszakmeister]

@tarruda Nope. My rb_insert/rb_remove support inserting at/removing from any position.

I'm just curious @ZyX-I, but that doesn't sound like a typical ring buffer behavior--it seems more list-like. Why do you want this in a ring buffer? Isn't it inefficient too--O(n) instead of O(1)--when not at the "end" (or "beginning" for remove) of the buffer? I'm sure you have your reasons, I'm just curious about the motivations.

[oni-link]

Does not work for the wrap around case:
If in the first half no NL and NUL are found, write_output() returns 0. So in the second (and last) iteration of the loop the first half is used again, but the second half is not used (possible NL here).
If a NUL was found in the first iteration, it would be changed to a NL and the next iteration would
output "something".

Is write_output() working correctly for eof == true, if the two buffer halfs are written not as one?

[ZyX-I]

@jszakmeister It is O(n), but these are simple memmoves. The reasoning is that I a) avoid a huge amount of malloc() calls and b) it should happen rather rarely. Another options considered:

• If I will use a double linked list I will have to do one additional malloc() per element.
• If I will use a simple array then I will have to memmove each time element is added at the end once buffer is filled.

Though one thing looks like being worth trying: double linked list based on a pre-allocated array. This is two malloc() calls per list (list itself + array of pointers to “free” list items) (can be refactored to one malloc() if absolutely needed) and O(1) for inserting/removing at any position. Did not came up with this idea until your question and ring buffer seemed more natural for history list.

ShaDa code needs iteration, insert/append, remove; NeoVim history needs iteration, append, remove, index (take element with given index). So for ShaDa linked list is completely fine (insert and remove positions are determined using iteration, so there is no difference between ring buffer and array here), for NeoVim it is O(n) index, but most of time nobody cares though: index is AFAIK for histget(, -x) only (histget(, +x) needs iteration). Maybe it is worth refactoring in both places.

[oni-link]

Can't we use rbuffer_read_ptr() instead of the RBUFFER_UNTIL_READ_OR_EMPTY macro?
handle_bracket_*() is otherwise not checked after something was consumed.

[oni-link]

termkey_push_bytes() could return a value bigger than input->read_buffer->size, so the following call to rbuffer_consumed() would fail.

[tarruda]

How can that happen? We are passing MIN(count, len) to termkey_push_bytes which should always be <= input->read_buffer->size

[oni-link]

If the termkey buffer gets full it will return (size_t)-1.

size_t termkey_push_bytes(TermKey *tk, const char *bytes, size_t len)
{
  if(tk->buffstart) {
    memmove(tk->buffer, tk->buffer + tk->buffstart, tk->buffcount);
    tk->buffstart = 0;
  }

  /* Not expecting it ever to be greater but doesn't hurt to handle that */
  if(tk->buffcount >= tk->buffsize) {
    errno = ENOMEM;
    return (size_t)-1;
  }

  if(len > tk->buffsize - tk->buffcount)
    len = tk->buffsize - tk->buffcount;

  // memcpy(), not strncpy() in case of null bytes in input
  memcpy(tk->buffer + tk->buffcount, bytes, len);
  tk->buffcount += len;

  return len;
}

[tarruda]

I'm not sure on how to handle this error, my best guess is to treat as an out of memory condition and exit the program. What do you think?

@leonerd, is this tk->buffcount >= tk->buffsize condition related to libtermkey failing to allocate memory at some point?

[leonerd]

Not so much failing, as simply trying to keep a bounded size. I felt it best not to grow the buffer arbitrarily.

[tarruda]

Not so much failing, as simply trying to keep a bounded size. I felt it best not to grow the buffer arbitrarily.

ok, but if the caller consumes all parsed keys right after every termkey_push_bytes call, then this condition is not possible right?