[tarantool-patches] [PATCH v5 2/9] lib: implement JSON tree class for json library
Vladimir Davydov
vdavydov.dev at gmail.com
Thu Nov 29 20:50:57 MSK 2018
On Thu, Nov 29, 2018 at 08:38:16PM +0300, Vladimir Davydov wrote:
> On Mon, Nov 26, 2018 at 03:53:03PM +0300, Kirill Shcherbatov wrote:
> > +int
> > +json_tree_add(struct json_tree *tree, struct json_token *parent,
> > + struct json_token *token)
> > +{
> > + if (parent == NULL)
> > + parent = &tree->root;
> > + uint32_t rolling_hash =
> > + json_token_hash(token, parent->rolling_hash);
> > + assert(json_tree_lookup(tree, parent, token) == NULL);
> > + uint32_t insert_idx = (token->key.type == JSON_TOKEN_NUM) ?
> > + (uint32_t)token->key.num - 1 :
> > + parent->child_size;
> > + if (insert_idx >= parent->child_size) {
> > + uint32_t new_size =
> > + parent->child_size == 0 ? 1 : 2 * parent->child_size;
> > + while (insert_idx >= new_size)
> > + new_size *= 2;
> > + struct json_token **children =
> > + realloc(parent->children, new_size*sizeof(void *));
> > + if (unlikely(children == NULL))
> > + return -1;
> > + memset(children + parent->child_size, 0,
> > + (new_size - parent->child_size)*sizeof(void *));
> > + parent->children = children;
> > + parent->child_size = new_size;
>
> Ouch, this looks much more complex than it was in the previous version.
> Please revert. Quoting myself:
>
> } > + if (insert_idx >= parent->children_count) {
> } > + uint32_t new_size = insert_idx + 1;
> }
> } We usually double the size with each allocation. If this is intentional,
> } please add a comment.
>
> I didn't push you to change that. I just wanted you to add a comment
> saying that we can afford quadratic algorithmic complexity here, because
> this function is far from a hot path and we expect the tree to be rather
> small.
Come to think of it, don't bother. Doubling the array will probably
increases chances that the allocation will stay close to the node
itself, and it looks saner in a common sense. Let's please just rename
child_size to child_count_max and increase the initial allocation up to
say 8, OK?
More information about the Tarantool-patches
mailing list