On 08 Jul 2018, at 3:31 PM, Yann Ylavic <ylavic....@gmail.com> wrote:
> Since the other apr_json_decode_*() functions are not thread-safe (and
> shouldn't be), couldn't we (re)use a single self->ptemp here (cleared
> before leaving)?
> The locking potentially done by apr_pool_create() looks unnecessary,
> or at least a single subpool for the mutually recursive calls to
> apr_json_decode_*() would be enough.
It’s a tradeoff, and I think it’s probably at it’s most efficient as it is. The
code is optimised for smallest RAM footprint, which makes sense. The more RAM
it uses the more likely it is to not fit in a CPU cache, with a corresponding
slowdown, especially in a webserver environment where there are a lot of
concurrent requests, and the parsed structures are likely to hang around until
the end of the request.
>> +
>> + if (self->p >= self->e) {
>> + status = APR_EOF;
>> + goto out;
>> + }
>
> Before creating the subpool above maybe?
It’s a failure case, but strictly yes it should be fixed.
>
>> +
>> + self->level--;
>> + if (self->level < 0) {
>> + return APR_EINVAL;
>> + }
>
> Ditto (here moreover the subpool leaks).
> Also possibly don't change the level if it's already <= 0?
Same here.
>> + {
>> + json_link_t *node;
>> + apr_array_header_t *array = apr_array_make(self->pool, count,
>> sizeof(apr_json_value_t *));
>> + for (node = head; node; node = node->next) {
>> + *((apr_json_value_t **) (apr_array_push(array))) = node->value;
>> + }
>> + *retval = array;
>> + }
>
> Hmm, finally we may not need to count the entries in a first loop
> (hence any subpool), couldn't we let the array auto-grow by itself in
> the loop and do validation + add in a single pass?
Again this is inefficient in terms of RAM footprint. Walking it twice is not a
problem, as it fits in a CPU cache, and we use this extra memory for the
shortest possible time.
> Same here:
> if (self->level <= 0)
> return APR_EINVAL;
> self->level—;
Makes sense.
>> + if ((status = apr_json_decode_value(self, &key)))
>> + goto out;
>
> Maybe apr_json_decode_string() where we'd require/validate the
> leading/trailing quotes?
Alas, that breaks whitespace handling.
Fixes above in r1835364.
Regards,
Graham
—
