Re: Minor optimization in json.c

[Philipp Stephani <p.stephani2@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6659 bytes --]

Eli Zaretskii <eliz@gnu.org> schrieb am So., 23. Sep. 2018 um 15:06 Uhr:

> > Date: Sun, 23 Sep 2018 12:06:38 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: emacs-devel@gnu.org
> >
> > Actually, I think an even better idea is to do this like
> > insert-file-contents does: insert the raw string into the gap, then
> > decode it in one go with decode_coding_gap.  I will publish a proposed
> > patch along these lines soon.
>
> Here it is.  Comments are welcome.
>

Thanks, just 2 stylistic comments.
1. Could you try to eliminate global mutable state (i.e. the
json_inserted_bytes global variable)? I know that with the current
threading implementation access is properly synchronized, but the global
variable is still a code smell and unnecessarily couples details of the
threading implementation with the JSON adapter code. I think you should be
able to move it into the json_insert_data or json_buffer_and_size
structures.
2. Could you try factoring out the buffer management code into new
functions in insdel.c with a simple interface? The details on how to keep
track of the buffer insertion status aren't really related to JSON, and it
would be better to keep json.c focused on providing the JSON adapter
(single responsibility principle). Maybe with an interface such as:
void begin_insert_utf8_string (void);
void insert_utf8_string (const char *chars, ptrdiff_t nbytes);
void finish_insert_utf8_string (ptrdiff_t nbytes);


>
> (I needed to make a change in one of the tests, because it not longer
> matches the code: where previously after-change-functions were invoked
> after each chunk of JSON was inserted, and thus were protected by
> internal_catch_all, they are now invoked only once at the end of the
> insertion, and are no longer protected.  Which means the effect of
> that protection cannot be easily tested from Lisp, AFAICT.)
>
> diff --git a/src/json.c b/src/json.c
> index 17cc096..7bef80f 100644
> --- a/src/json.c
> +++ b/src/json.c
> @@ -626,15 +626,25 @@ struct json_buffer_and_size
>    ptrdiff_t size;
>  };
>
> +/* This tracks how many bytes were inserted by the callback since
> +   json_dump_callback was called.  */
> +static ptrdiff_t json_inserted_bytes;
> +
>  static Lisp_Object
>  json_insert (void *data)
>  {
> +  ptrdiff_t gap_size = GAP_SIZE - json_inserted_bytes;
>    struct json_buffer_and_size *buffer_and_size = data;
> -  /* FIXME: This should be possible without creating an intermediate
> -     string object.  */
> -  Lisp_Object string
> -    = json_make_string (buffer_and_size->buffer, buffer_and_size->size);
> -  insert1 (string);
> +  ptrdiff_t len = buffer_and_size->size;
> +
> +  /* Enlarge the gap if necessary.  */
> +  if (gap_size < len)
> +    make_gap (len - gap_size);
> +
> +  /* Copy this chunk of data into the gap.  */
> +  memcpy ((char *) BEG_ADDR + PT_BYTE - BEG_BYTE + json_inserted_bytes,
> +         buffer_and_size->buffer, len);
> +  json_inserted_bytes += len;
>    return Qnil;
>  }
>
> @@ -695,10 +705,15 @@ usage: (json-insert OBJECT &rest ARGS)  */)
>    json_t *json = lisp_to_json (args[0], &conf);
>    record_unwind_protect_ptr (json_release_object, json);
>
> +  prepare_to_modify_buffer (PT, PT, NULL);
> +  move_gap_both (PT, PT_BYTE);
> +  json_inserted_bytes = 0;
>    struct json_insert_data data;
>    /* If desired, we might want to add the following flags:
>       JSON_DECODE_ANY, JSON_ALLOW_NUL.  */
>    int status
> +    /* Could have used json_dumpb, but that became available only in
> +       Jansson 2.10, whereas we want to support 2.7 and upward.  */
>      = json_dump_callback (json, json_insert_callback, &data,
> JSON_COMPACT);
>    if (status == -1)
>      {
> @@ -708,6 +723,61 @@ usage: (json-insert OBJECT &rest ARGS)  */)
>          json_out_of_memory ();
>      }
>
> +  if (json_inserted_bytes > 0)
> +    {
> +      ptrdiff_t inserted = 0;
> +
> +      /* Make the inserted text part of the buffer, as unibyte text.  */
> +      GAP_SIZE -= json_inserted_bytes;
> +      GPT      += json_inserted_bytes;
> +      GPT_BYTE += json_inserted_bytes;
> +      ZV       += json_inserted_bytes;
> +      ZV_BYTE  += json_inserted_bytes;
> +      Z        += json_inserted_bytes;
> +      Z_BYTE   += json_inserted_bytes;
> +
> +      if (GAP_SIZE > 0)
> +       /* Put an anchor to ensure multi-byte form ends at gap.  */
> +       *GPT_ADDR = 0;
> +
> +      /* Decode the stuff we've read into the gap.  */
> +      struct coding_system coding;
> +      memset (&coding, 0, sizeof (coding));
> +      setup_coding_system (Qutf_8_unix, &coding);
> +      coding.dst_multibyte =
> +       !NILP (BVAR (current_buffer, enable_multibyte_characters));
> +      if (CODING_MAY_REQUIRE_DECODING (&coding))
> +       {
> +         move_gap_both (PT, PT_BYTE);
> +         GAP_SIZE += json_inserted_bytes;
> +         ZV_BYTE -= json_inserted_bytes;
> +         Z_BYTE -= json_inserted_bytes;
> +         ZV -= json_inserted_bytes;
> +         Z -= json_inserted_bytes;
> +         decode_coding_gap (&coding, json_inserted_bytes,
> json_inserted_bytes);
> +         inserted = coding.produced_char;
> +       }
> +      else
> +       {
> +         invalidate_buffer_caches (current_buffer,
> +                                   PT, PT + json_inserted_bytes);
> +         adjust_after_insert (PT, PT_BYTE,
> +                              PT + json_inserted_bytes,
> +                              PT_BYTE + json_inserted_bytes,
> +                              json_inserted_bytes);
> +         inserted = json_inserted_bytes;
> +       }
> +
> +      /* Call after-change hooks if necessary.  */
> +      if (inserted > 0)
> +       {
> +         signal_after_change (PT, 0, inserted);
> +         update_compositions (PT, PT, CHECK_BORDER);
> +       }
> +
> +      /* Move point to after the inserted text.  */
> +      SET_PT_BOTH (PT + inserted, PT_BYTE + json_inserted_bytes);
> +    }
>    return unbind_to (count, Qnil);
>  }
>
> diff --git a/test/src/json-tests.el b/test/src/json-tests.el
> index 911bc49..bffee8f 100644
> --- a/test/src/json-tests.el
> +++ b/test/src/json-tests.el
> @@ -272,10 +272,11 @@ 'json-tests--error
>                    (cl-incf calls)
>                    (throw 'test-tag 'throw-value))
>                  :local)
> -      (should-error
> -       (catch 'test-tag
> -         (json-insert '((a . "b") (c . 123) (d . [1 2 t :false]))))
> -       :type 'no-catch)
> +      (should
> +       (equal
> +        (catch 'test-tag
> +          (json-insert '((a . "b") (c . 123) (d . [1 2 t :false]))))
> +        'throw-value))
>        (should (equal calls 1)))))
>
>  (ert-deftest json-serialize/bignum ()
>

[-- Attachment #2: Type: text/html, Size: 8221 bytes --]