Repository navigation
Conversation
I checked the entire codebase, and every place that vstr_init_len was called, there was a call to mp_obj_new_str_from_vstr after it. mp_obj_new_str_from_vstr always tries to reallocate a new buffer 1 byte larger than the original to store the terminating null character. In many cases, if we allocated the initial buffer to be 1 byte longer, we can prevent this extra allocation, and just reuse the originally allocated buffer. Asking to read 256 bytes and only getting 100 will still cause the extra allocation, but if you ask to read 256 and get 256 then the extra allocation will be optimized away. Yes - the reallocation is optimized in the heap to try and reuse the buffer if it can, but it takes quite a few cycles to figure this out.
|
Nice, looks good to me and makes sense. |
The problem with this is that vstr is a generic buffer object. It's nice that in every place you saw, vstr_init_len is followed by mp_obj_new_str_from_vstr. But it doesn't preclude some other code to request vstr_init_len(256) and expect that it will take exactly 256 bytes, and not for example 256+16, factory memory manager overhead. I proposed a solution to that: we split current vstr into vstr for strings and vbuf for generic buffers. But @dpgeorge argued of bloat and scope creep (and I can make an argument that it's paradigmatically right, not that it's needed). All in all, I'm +0.5 on this, but @dpgeorge should look into this as he last refactored vstr code (and before that, I did guerrilla patches to it to factor in various adhoc improvements, like this one, which he eventually had to refactor/clean up). |
|
If you really don't want the extra byte, you can just call vstr_init instead of vstr_init_len. I was originally going to introduce a new API, which I called: vstr_init_alloc_plus_1, but by the time I was done, I had changed every single instance of vstr_init_len to vstr_init_alloc_plus_1, so I left out the rename of the new API. I think that this should actually be a performance improvement, but I couldn't figure out what performance tests we run, and when I tried run-bench-tests it failed. |
|
Merged in 9f76dcd with a change to the comment above vstr_init_len. The vstr_t type now (after previous patches here and there) behaves pretty much like a vbuf_t, and it has 2 convenience functions for actual string (ie null terminated buffer) behaviour:
So that's how to think about and use the vstr_t type (primarily as a buffer with 2 str convenience functions). We could rename vstr to vbuf but that would be 630 lines changed. We could change vstr_init_len to something more appropriate, like vstr_init_str_len. If you want to init a normal buffer use vstr_init, not vstr_init_len. |
Increase clone depth. Fixes micropython#1247
5 participants
I checked the entire codebase, and every place that vstr_init_len
was called, there was a call to mp_obj_new_str_from_vstr after it.
mp_obj_new_str_from_vstr always tries to reallocate a new buffer
1 byte larger than the original to store the terminating null
character.
In many cases, if we allocated the initial buffer to be 1 byte
longer, we can prevent this extra allocation, and just reuse
the originally allocated buffer.
Asking to read 256 bytes and only getting 100 will still cause
the extra allocation, but if you ask to read 256 and get 256
then the extra allocation will be optimized away.
Yes - the reallocation is optimized in the heap to try and reuse
the buffer if it can, but it takes quite a few cycles to figure
this out.