search

msgpack / msgpack-ruby

MessagePack implementation for Ruby / msgpack.org[Ruby]
http://msgpack.org/
Apache License 2.0
764 stars 117 forks source link

Better fix for marking buffers held on the stack #349

Closed casperisfine closed 1 year ago

casperisfine commented 1 year ago

Followup: https://github.com/msgpack/msgpack-ruby/pull/347

As discussed previously, the alloca hack was just a quick fix, it's not good as it could lead to stack overflows and also could be optimized away by compilers etc.

Here instead we copy all the VALUE references into a dedicated object that is in change of holding and pinning these references.

cc @peterzhu2118 @eregon @pavel-workato @sitano

eregon commented 1 year ago

Thanks!

I was thinking maybe we can just use a msgpack Buffer to hold onto the struct and the VALUEs inside. But probably that doesn't quite work and hence the new class?

byroot commented 1 year ago

But probably that doesn't quite work and hence the new class?

Yeah that doesn't. The buffer struct does this weird thing that it's a linked list of buffer_chunk_t, but the tail of the list is a member of the butter_t. So if you copy the buffer elsewhere, the tail reference is broken...

From what I could gather, it's an optimization to avoid pointer chasing when you append to the buffer, but it's unclear how efficient that really is.

We talked with @peterzhu2118 about potentially simplyfing the buffer implementation, but not sure when we'll actually get around it.