[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: JSON/YAML/TOML/etc. parsing performance

From: Paul Eggert
Subject: Re: JSON/YAML/TOML/etc. parsing performance
Date: Tue, 3 Oct 2017 13:52:54 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0

On 10/03/2017 05:26 AM, Philipp Stephani wrote:

> +#if __has_attribute (warn_unused_result)
> +# define ATTRIBUTE_WARN_UNUSED_RESULT __attribute__ ((__warn_unused_result__))
> +#else
> +#endif

Hmm... why do we need this attribute?  You use it with 2 static
functions, so this sounds like a left-over from the development stage?

It's not strictly needed (and if you don't like it, I can remove it), but it helps catching memory leaks.

I've found __warn_unused_result__ to be more trouble than it's worth in library functions. Emacs has lib/ignore-value.h in order to work around __warn_unused_result__ brain damage, for example. For static functions the problem is less, but still, I mildly favor leaving it out.

    And btw, how can size be greater than SIZE_MAX in this case? This is
    a valid Lisp object, isn't it?  (There are more such tests in the
    patch, e.g. in lisp_to_json, and I think they, too, are redundant.)

Depends on the range of ptrdiff_t and size_t. IIUC nothing in the C standard guarantees PTRDIFF_MAX <= SIZE_MAX. If we want to guarantee that, we should probably add at least a static assertion.

There should be no need for that. No Lisp object can exceed min (PTRDIFF_MAX, SIZE_MAX) bytes; alloc.c guarantees this, so that Emacs should work even on oddball platforms where SIZE_MAX < PTRDIFF_MAX, and there is no need for a runtime check here.

The main practical problem here, by the way, is objects whose sizes exceed PTRDIFF_MAX on mainstream 32-bit platforms. This Does Not Work because you cannot subtract pointers within such objects reliably, sometimes even when the true difference is representable as ptrdiff_t! This is the main reason alloc.c prohibits creating such large objects, and that Emacs should reject any attempt to support such objects (even non-Lisp objects).

> +  if (buffer_and_size->size > PTRDIFF_MAX)
> +    xsignal1 (Qoverflow_error, build_string ("buffer too large"));
> +  insert (buffer_and_size->buffer, buffer_and_size->size);

I don't think we need this test here, as 'insert' already has the
equivalent test in one of its subroutines.

It can't, because it takes the byte length as ptrdiff_t. We need to check before whether the size is actually in the valid range of ptrdiff_t.

A PTRDIFF_MAX test is needed if the JSON library can return strings longer than PTRDIFF_MAX. Please just use buffer_overflow () to signal the error, though.

> +    case JSON_INTEGER:
> +      {
> +        json_int_t value = json_integer_value (json);
> +        if (FIXNUM_OVERFLOW_P (value))
> +          xsignal1 (Qoverflow_error,
> +                    build_string ("JSON integer is too large"));
> +        return make_number (value);

This overflow test is also redundant, as make_number already does it.

It can't, because json_int_t can be larger than EMACS_INT. Also, make_number doesn't contain any checks.

You're right that a test is needed. However, elsewhere we just use xsignal0 (Qoverflow_error) for this sort of thing, and I suggest being consistent and doing that here as well. Similarly for other calls to xsignal1 with Qoverflow_error.

> +    case JSON_STRING:
> +      {
> +        size_t size = json_string_length (json);
> +        if (FIXNUM_OVERFLOW_P (size))
> +          xsignal1 (Qoverflow_error, build_string ("JSON string is too long"));
> +        return json_make_string (json_string_value (json), size);

Once again, the overflow test is redundant, as make_specified_string
(called by json_make_string) already includes an equivalent test.

And once again, we need to check at least whether the size fits into ptrdiff_t.
You're right, a test is needed. However, I suggest using string_overflow () to signal string overflows.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]