Minor optimization in json.c

Minor optimization in json.c

[Eli Zaretskii]

Currently, json.c has this FIXME:

  /* 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);

IIUC the issue, the change below should fix this.  All the tests in
json-tests.el pass after the change, but maybe you had failures in
other scenarios?  If not, I think we should make this change.

diff --git a/src/json.c b/src/json.c
index 17cc096..20280d8 100644
--- a/src/json.c
+++ b/src/json.c
@@ -630,11 +630,29 @@ static Lisp_Object
 json_insert (void *data)
 {
   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);
+  struct coding_system coding;
+  ptrdiff_t bytes = buffer_and_size->size, chars = bytes, opoint;
+
+  memset (&coding, 0, sizeof (coding));
+  /* JSON strings are UTF-8 strings.  */
+  setup_coding_system (Qutf_8_unix, &coding);
+  coding.source = buffer_and_size->buffer;
+  coding.mode |= CODING_MODE_LAST_BLOCK;
+  /* JSON strings are unibyte strings.  */
+  coding.src_chars = coding.src_bytes = bytes;
+
+  /* Call before-change hooks.  */
+  prepare_to_modify_buffer (PT, PT, NULL);
+  decode_coding_object (&coding, Qnil, 0, 0, chars, bytes, Fcurrent_buffer ());
+  /* Update buffer's point due to insertion of the string.  */
+  SET_BUF_PT_BOTH (current_buffer,
+		   PT + coding.produced_char, PT_BYTE + coding.produced);
+
+  /* Call after-change hooks.  */
+  opoint = PT - coding.produced_char;
+  signal_after_change (opoint, 0, coding.produced_char);
+  update_compositions (opoint, PT, CHECK_BORDER);
+
   return Qnil;
 }
 

Re: Minor optimization in json.c

[Eli Zaretskii]

> Date: Sat, 22 Sep 2018 17:10:03 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> Currently, json.c has this FIXME:
> 
>   /* 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);
> 
> IIUC the issue, the change below should fix this.  All the tests in
> json-tests.el pass after the change, but maybe you had failures in
> other scenarios?  If not, I think we should make this change.

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.

Re: Minor optimization in json.c

[Eli Zaretskii]

> 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.

(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 ()

Re: Minor optimization in json.c

[Philipp Stephani]

[-- 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 --]

Re: Minor optimization in json.c

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 3 Oct 2018 21:57:24 +0200
> Cc: emacs-devel@gnu.org
> 
> Thanks, just 2 stylistic comments.

Thanks for reviewing the code.

> 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.

I did that, but it needed adding new members to 2 data structures, and
copying the values back and forth between them.  Not sure this is more
elegant.

> 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 didn't do this.  I don't like breaking code into functions that have
no meaning on their own, especially when there's only one caller.  (We
do something similar in insert-file-contents, but the structure of the
code and the details are sufficiently different that I decided not to
make a single implementation that would support both.)

I also don't agree with the argument about tracking the insertion
status not being related to JSON: we do similar stuff in many places,
and even json.c itself has similarly "unrelated" code, like the one
under CONSP (lisp) in lisp_to_json_toplevel_1.

The changes are now pushed to master.

Re: Minor optimization in json.c

[Tom Tromey]

>>>>> "Eli" == Eli Zaretskii <eliz@gnu.org> writes:

>> 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.

Eli> I did that, but it needed adding new members to 2 data structures, and
Eli> copying the values back and forth between them.  Not sure this is more
Eli> elegant.

It's also possible to put this sort of thing into the thread object.

Tom