Refactor unpacking of recursive types #372
Conversation
Extracted from: #370 As initially implemented, we allocate a new stack whenever we encounter a child stack. This means acquiring a whole 4kiB chunk from the arena, which generally is overkill. Instead we can introduce a new type of stack item, that behave just like a Hash or Array.
Now that there's no longer a concept of child stacks, we can embed the struct and save a malloc/free pair.
Reduce its size from 264 to 256B.
| child_stack->parent = uk->stack; | ||
| uk->stack = child_stack; | ||
|
|
||
| _msgpack_unpacker_stack_push(uk, STACK_TYPE_RECURSIVE, 1, Qnil); |
There was a problem hiding this comment.
Since a child no longer creating a new stack, does this change mean that recursive types will trigger a stack overflow much sooner than before?
There was a problem hiding this comment.
Possibly, yes. But the stack size if 128, that's quite a deep document, I don't think that's something you are likely to hit..
That said, your comment make me realize there's just no check whatsoever for the stack size :/
Would be worth adding one.
There was a problem hiding this comment.
Nevermind, there's a check for it:
if(uk->stack.capacity - uk->stack.depth <= 0) {
return PRIMITIVE_STACK_TOO_DEEP;
}| int raised; | ||
| obj = protected_proc_call(proc, 1, &uk->self, &raised); | ||
| uk->stack = child_stack->parent; | ||
| _msgpack_unpacker_free_stack(child_stack); | ||
| msgpack_unpacker_stack_pop(uk); |
There was a problem hiding this comment.
Can we assert that the pop here is actually popping a STACK_TYPE_RECURSIVE to ensure that we don't have a stack inconsistency issue?
There was a problem hiding this comment.
We don't quite have a good setup for asserts in the gem, but yes that would be valuable to put something like that in place.
Extracted from: #370
As initially implemented, we allocate a new stack whenever we encounter a recursive type. This means acquiring a whole 4kiB chunk from the arena, which generally is overkill.
Instead we can introduce a new type of stack item, that behave just like a Hash or Array.