bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Pip Cet]

Playing around with the WebAssembly port, I noticed that pdumper, in
creating the dump file, makes way too many syscalls: it uses
emacs_write(), not fwrite(), so these calls translate to actual
syscalls and context switches. On immature systems (or in special
circumstances like a device mounted synchronously), they might
actually cause a hardware write for each syscall, which would wear out
flash quickly and be generally wasteful.

I've looked into the problem, and it seems easy to solve and worth it
in terms of debuggability and performance.

Patch will be attached once this has a bug number.

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Pip Cet]

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

On Tue, Mar 2, 2021 at 8:35 PM Pip Cet <pipcet@gmail.com> wrote:
> I've looked into the problem, and it seems easy to solve and worth it
> in terms of debuggability and performance.

Very rough benchmarks, but this seems to be clearly worth it:

Performance:
With patch:
real    0m3.861s
user    0m3.776s
sys    0m0.085s

Without patch:
real    0m7.001s
user    0m4.476s
sys    0m2.511s

Number of syscalls:
With patch: 415442
Without patch: 2028307

> Patch will be attached once this has a bug number.

And here's the patch. Testing would be very appreciated.

I'm unsure about the precise usage of dump_off vs ptrdiff_t here; I
don't think it matters, but suggestions, nitpicks, and comments, on
this or any other aspect, would be very appreciated.

Pip

[-- Attachment #2: 0001-Prepare-pdumper-dump-file-in-memory-write-it-in-one-.patch --]
[-- Type: text/x-patch, Size: 2733 bytes --]

From 92ee138852b34ede2f43dd7f93f310fc746bb3bf Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Tue, 2 Mar 2021 20:38:23 +0000
Subject: [PATCH] Prepare pdumper dump file in memory, write it in one go
 (Bug#46881)

* src/pdumper.c (struct dump_context): Add buf, buf_size, max_offset fields.
(grow_buffer): New function.
(dump_write): Use memcpy, not an actual emacs_write.
(dump_seek): Keep track of maximum seen offset.
(Fdump_emacs_portable): Write out the file contents when done.
---
 src/pdumper.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/src/pdumper.c b/src/pdumper.c
index 337742fda4ade..62ddad8ee5e34 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -473,6 +473,10 @@ dump_fingerprint (char const *label,
 {
   /* Header we'll write to the dump file when done.  */
   struct dump_header header;
+  /* Data that will be written to the dump file.  */
+  void *buf;
+  ptrdiff_t buf_size;
+  ptrdiff_t max_offset;
 
   Lisp_Object old_purify_flag;
   Lisp_Object old_post_gc_hook;
@@ -581,6 +585,13 @@ dump_fingerprint (char const *label,
 \f
 /* Dump file creation */
 
+static void dump_grow_buffer (struct dump_context *ctx)
+{
+  ctx->buf = xrealloc (ctx->buf, ctx->buf_size = (ctx->buf_size ?
+						  (ctx->buf_size * 2)
+						  : 1024 * 1024));
+}
+
 static dump_off dump_object (struct dump_context *ctx, Lisp_Object object);
 static dump_off dump_object_for_offset (struct dump_context *ctx,
 					Lisp_Object object);
@@ -747,8 +758,9 @@ dump_write (struct dump_context *ctx, const void *buf, dump_off nbyte)
   eassert (nbyte == 0 || buf != NULL);
   eassert (ctx->obj_offset == 0);
   eassert (ctx->flags.dump_object_contents);
-  if (emacs_write (ctx->fd, buf, nbyte) < nbyte)
-    report_file_error ("Could not write to dump file", ctx->dump_filename);
+  while (ctx->offset + nbyte > ctx->buf_size)
+    dump_grow_buffer (ctx);
+  memcpy ((char *)ctx->buf + ctx->offset, buf, nbyte);
   ctx->offset += nbyte;
 }
 
@@ -828,6 +840,8 @@ dump_tailq_pop (struct dump_tailq *tailq)
 static void
 dump_seek (struct dump_context *ctx, dump_off offset)
 {
+  if (ctx->max_offset < ctx->offset)
+    ctx->max_offset = ctx->offset;
   eassert (ctx->obj_offset == 0);
   if (lseek (ctx->fd, offset, SEEK_SET) < 0)
     report_file_error ("Setting file position",
@@ -4159,6 +4173,8 @@ DEFUN ("dump-emacs-portable",
   ctx->header.magic[0] = dump_magic[0];
   dump_seek (ctx, 0);
   dump_write (ctx, &ctx->header, sizeof (ctx->header));
+  if (emacs_write (ctx->fd, ctx->buf, ctx->max_offset) < ctx->max_offset)
+    report_file_error ("Could not write to dump file", ctx->dump_filename);
 
   dump_off
     header_bytes = header_end - header_start,
-- 
2.30.1

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Alan Third]

On Tue, Mar 02, 2021 at 08:45:04PM +0000, Pip Cet wrote:
> On Tue, Mar 2, 2021 at 8:35 PM Pip Cet <pipcet@gmail.com> wrote:
> > I've looked into the problem, and it seems easy to solve and worth it
> > in terms of debuggability and performance.
> 
> Very rough benchmarks, but this seems to be clearly worth it:
> 
> Performance:
> With patch:
> real    0m3.861s
> user    0m3.776s
> sys    0m0.085s
> 
> Without patch:
> real    0m7.001s
> user    0m4.476s
> sys    0m2.511s
> 
> Number of syscalls:
> With patch: 415442
> Without patch: 2028307

My quick test on macOS by doing:

rm src/*.pdmp
time make

sees it going from ~26s without patch to ~10s with patch, so a
considerable improvement.

> > Patch will be attached once this has a bug number.
> 
> And here's the patch. Testing would be very appreciated.

It appears to work fine here, but I don't know if there's anything
specific to test other than just running Emacs.
-- 
Alan Third

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Pip Cet]

> My quick test on macOS by doing:
>
> rm src/*.pdmp
> time make
>
> sees it going from ~26s without patch to ~10s with patch, so a
> considerable improvement.

Thanks for testing!

> > > Patch will be attached once this has a bug number.
> >
> > And here's the patch. Testing would be very appreciated.
>
> It appears to work fine here, but I don't know if there's anything
> specific to test other than just running Emacs.

I suspect there may be problems on systems with very little memory. Do
you have an easy way to determine maximum resident size (on Debian
GNU/Linux, /bin/time works)? It would be interesting to see if that's
actually different.

Thanks again!
Pip

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Alan Third]

On Wed, Mar 03, 2021 at 07:10:28AM +0000, Pip Cet wrote:
> > My quick test on macOS by doing:
> >
> > rm src/*.pdmp
> > time make
> >
> > sees it going from ~26s without patch to ~10s with patch, so a
> > considerable improvement.
> 
> Thanks for testing!
> 
> > > > Patch will be attached once this has a bug number.
> > >
> > > And here's the patch. Testing would be very appreciated.
> >
> > It appears to work fine here, but I don't know if there's anything
> > specific to test other than just running Emacs.
> 
> I suspect there may be problems on systems with very little memory. Do
> you have an easy way to determine maximum resident size (on Debian
> GNU/Linux, /bin/time works)? It would be interesting to see if that's
> actually different.

I tried using time -l the same as above and both came  out with
roughly the same values, but I suspect that's probably just me getting
the values for running make.

Is there a better way of testing dumping?
-- 
Alan Third

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Pip Cet]

On Wed, Mar 3, 2021 at 7:58 PM Alan Third <alan@idiocy.org> wrote:
> On Wed, Mar 03, 2021 at 07:10:28AM +0000, Pip Cet wrote:
> > > My quick test on macOS by doing:
> > >
> > > rm src/*.pdmp
> > > time make
> > >
> > > sees it going from ~26s without patch to ~10s with patch, so a
> > > considerable improvement.
> >
> > Thanks for testing!
> >
> > > > > Patch will be attached once this has a bug number.
> > > >
> > > > And here's the patch. Testing would be very appreciated.
> > >
> > > It appears to work fine here, but I don't know if there's anything
> > > specific to test other than just running Emacs.
> >
> > I suspect there may be problems on systems with very little memory. Do
> > you have an easy way to determine maximum resident size (on Debian
> > GNU/Linux, /bin/time works)? It would be interesting to see if that's
> > actually different.
>
> I tried using time -l the same as above and both came  out with
> roughly the same values, but I suspect that's probably just me getting
> the values for running make.

Thanks.

> Is there a better way of testing dumping?

I guess you could run "time  ./temacs --batch  -l loadup
--temacs=pbootstrap" directly (in src/)...

I realize that's not an answer to your question, but IME, dumping bugs
will often lead to immediate failures to bootstrap, whereas GC bugs
are tricky and don't. Since this bug doesn't affect GC in any way, I
think we've done enough initial testing.

Pip

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Tue, 2 Mar 2021 20:45:04 +0000
> 
> On Tue, Mar 2, 2021 at 8:35 PM Pip Cet <pipcet@gmail.com> wrote:
> > I've looked into the problem, and it seems easy to solve and worth it
> > in terms of debuggability and performance.
> 
> Very rough benchmarks, but this seems to be clearly worth it:
> 
> Performance:
> With patch:
> real    0m3.861s
> user    0m3.776s
> sys    0m0.085s
> 
> Without patch:
> real    0m7.001s
> user    0m4.476s
> sys    0m2.511s
> 
> Number of syscalls:
> With patch: 415442
> Without patch: 2028307
> 
> > Patch will be attached once this has a bug number.
> 
> And here's the patch. Testing would be very appreciated.
> 
> I'm unsure about the precise usage of dump_off vs ptrdiff_t here; I
> don't think it matters, but suggestions, nitpicks, and comments, on
> this or any other aspect, would be very appreciated.

> From 92ee138852b34ede2f43dd7f93f310fc746bb3bf Mon Sep 17 00:00:00 2001
> From: Pip Cet <pipcet@gmail.com>
> Date: Tue, 2 Mar 2021 20:38:23 +0000
> Subject: [PATCH] Prepare pdumper dump file in memory, write it in one go
>  (Bug#46881)
> 
> * src/pdumper.c (struct dump_context): Add buf, buf_size, max_offset fields.
> (grow_buffer): New function.
> (dump_write): Use memcpy, not an actual emacs_write.
> (dump_seek): Keep track of maximum seen offset.
> (Fdump_emacs_portable): Write out the file contents when done.
> ---
>  src/pdumper.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/src/pdumper.c b/src/pdumper.c
> index 337742fda4ade..62ddad8ee5e34 100644
> --- a/src/pdumper.c
> +++ b/src/pdumper.c
> @@ -473,6 +473,10 @@ dump_fingerprint (char const *label,
>  {
>    /* Header we'll write to the dump file when done.  */
>    struct dump_header header;
> +  /* Data that will be written to the dump file.  */
> +  void *buf;
> +  ptrdiff_t buf_size;
> +  ptrdiff_t max_offset;
>  
>    Lisp_Object old_purify_flag;
>    Lisp_Object old_post_gc_hook;
> @@ -581,6 +585,13 @@ dump_fingerprint (char const *label,
>  \f
>  /* Dump file creation */
>  
> +static void dump_grow_buffer (struct dump_context *ctx)
> +{
> +  ctx->buf = xrealloc (ctx->buf, ctx->buf_size = (ctx->buf_size ?
> +						  (ctx->buf_size * 2)
> +						  : 1024 * 1024));
> +}
> +
>  static dump_off dump_object (struct dump_context *ctx, Lisp_Object object);
>  static dump_off dump_object_for_offset (struct dump_context *ctx,
>  					Lisp_Object object);
> @@ -747,8 +758,9 @@ dump_write (struct dump_context *ctx, const void *buf, dump_off nbyte)
>    eassert (nbyte == 0 || buf != NULL);
>    eassert (ctx->obj_offset == 0);
>    eassert (ctx->flags.dump_object_contents);
> -  if (emacs_write (ctx->fd, buf, nbyte) < nbyte)
> -    report_file_error ("Could not write to dump file", ctx->dump_filename);
> +  while (ctx->offset + nbyte > ctx->buf_size)
> +    dump_grow_buffer (ctx);
> +  memcpy ((char *)ctx->buf + ctx->offset, buf, nbyte);
>    ctx->offset += nbyte;
>  }
>  
> @@ -828,6 +840,8 @@ dump_tailq_pop (struct dump_tailq *tailq)
>  static void
>  dump_seek (struct dump_context *ctx, dump_off offset)
>  {
> +  if (ctx->max_offset < ctx->offset)
> +    ctx->max_offset = ctx->offset;
>    eassert (ctx->obj_offset == 0);
>    if (lseek (ctx->fd, offset, SEEK_SET) < 0)
>      report_file_error ("Setting file position",
> @@ -4159,6 +4173,8 @@ DEFUN ("dump-emacs-portable",
>    ctx->header.magic[0] = dump_magic[0];
>    dump_seek (ctx, 0);
>    dump_write (ctx, &ctx->header, sizeof (ctx->header));
> +  if (emacs_write (ctx->fd, ctx->buf, ctx->max_offset) < ctx->max_offset)
> +    report_file_error ("Could not write to dump file", ctx->dump_filename);
>  
>    dump_off
>      header_bytes = header_end - header_start,
> -- 
> 2.30.1

Thanks.

Daniel, Paul: any comments?  In particular, is it safe to allocate
large amounts of memory off the heap while dumping?  A couple of
places in pdumper.c says some parts of code should call malloc.

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Pip Cet]

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

On Wed, Mar 3, 2021 at 5:51 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Tue, 2 Mar 2021 20:45:04 +0000
> >
> > On Tue, Mar 2, 2021 at 8:35 PM Pip Cet <pipcet@gmail.com> wrote:
> > > I've looked into the problem, and it seems easy to solve and worth it
> > > in terms of debuggability and performance.

Since debuggability is such a concern, we probably shouldn't leak the
buffer memory. Revised patch attached. (This patch also removes the
lseek() syscalls; while not quite as numerous as the read() ones,
those did clutter up straces here).

> In particular, is it safe to allocate
> large amounts of memory off the heap while dumping?

Even if it isn't, we'd still be faster re-running the dump after
growing the dumper image than the current approach is.

>A couple of
> places in pdumper.c says some parts of code should call malloc.

IIUC, the prohibition on calling malloc, if it is still a concern,
applies only when loading the dump, not while writing it.

My main concern is the possibility of a partly-written dump file,
since we no longer turn "!UMPEDGNUEMACS" into "DUMPEDGNUEMACS" after
the dump. Maybe it would make sense to restore that feature?

Pip

[-- Attachment #2: 0001-Prepare-pdumper-dump-file-in-memory-write-it-in-one-.patch --]
[-- Type: text/x-patch, Size: 2932 bytes --]

From fae67c02955a5bbea16d554b8e735dc8bef6a9e2 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Tue, 2 Mar 2021 20:38:23 +0000
Subject: [PATCH] Prepare pdumper dump file in memory, write it in one go
 (Bug#46881)

* src/pdumper.c (struct dump_context): Add buf, buf_size, max_offset fields.
(dump_grow_buffer): New function.
(dump_write): Use memcpy, not an actual emacs_write.
(dump_seek): Keep track of maximum seen offset. Don't actually seek.
(Fdump_emacs_portable): Write out the file contents when done.
---
 src/pdumper.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/src/pdumper.c b/src/pdumper.c
index 337742fda4ade..29d1cb862e07f 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -473,6 +473,10 @@ dump_fingerprint (char const *label,
 {
   /* Header we'll write to the dump file when done.  */
   struct dump_header header;
+  /* Data that will be written to the dump file.  */
+  void *buf;
+  dump_off buf_size;
+  dump_off max_offset;
 
   Lisp_Object old_purify_flag;
   Lisp_Object old_post_gc_hook;
@@ -581,6 +585,13 @@ dump_fingerprint (char const *label,
 \f
 /* Dump file creation */
 
+static void dump_grow_buffer (struct dump_context *ctx)
+{
+  ctx->buf = xrealloc (ctx->buf, ctx->buf_size = (ctx->buf_size ?
+						  (ctx->buf_size * 2)
+						  : 8 * 1024 * 1024));
+}
+
 static dump_off dump_object (struct dump_context *ctx, Lisp_Object object);
 static dump_off dump_object_for_offset (struct dump_context *ctx,
 					Lisp_Object object);
@@ -747,8 +758,9 @@ dump_write (struct dump_context *ctx, const void *buf, dump_off nbyte)
   eassert (nbyte == 0 || buf != NULL);
   eassert (ctx->obj_offset == 0);
   eassert (ctx->flags.dump_object_contents);
-  if (emacs_write (ctx->fd, buf, nbyte) < nbyte)
-    report_file_error ("Could not write to dump file", ctx->dump_filename);
+  while (ctx->offset + nbyte > ctx->buf_size)
+    dump_grow_buffer (ctx);
+  memcpy ((char *)ctx->buf + ctx->offset, buf, nbyte);
   ctx->offset += nbyte;
 }
 
@@ -828,10 +840,9 @@ dump_tailq_pop (struct dump_tailq *tailq)
 static void
 dump_seek (struct dump_context *ctx, dump_off offset)
 {
+  if (ctx->max_offset < ctx->offset)
+    ctx->max_offset = ctx->offset;
   eassert (ctx->obj_offset == 0);
-  if (lseek (ctx->fd, offset, SEEK_SET) < 0)
-    report_file_error ("Setting file position",
-                       ctx->dump_filename);
   ctx->offset = offset;
 }
 
@@ -4159,6 +4170,12 @@ DEFUN ("dump-emacs-portable",
   ctx->header.magic[0] = dump_magic[0];
   dump_seek (ctx, 0);
   dump_write (ctx, &ctx->header, sizeof (ctx->header));
+  if (emacs_write (ctx->fd, ctx->buf, ctx->max_offset) < ctx->max_offset)
+    report_file_error ("Could not write to dump file", ctx->dump_filename);
+  xfree (ctx->buf);
+  ctx->buf = NULL;
+  ctx->buf_size = 0;
+  ctx->max_offset = 0;
 
   dump_off
     header_bytes = header_end - header_start,
-- 
2.30.1

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Lars Ingebrigtsen]

Pip Cet <pipcet@gmail.com> writes:

> Since debuggability is such a concern, we probably shouldn't leak the
> buffer memory. Revised patch attached. (This patch also removes the
> lseek() syscalls; while not quite as numerous as the read() ones,
> those did clutter up straces here).

I've tried the patch on a couple of systems here, and the resulting
Emacs works fine (as expected), and the pdumping is significantly faster
here, too.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Paul Eggert]

On 3/2/21 11:35 PM, Pip Cet wrote:
> IIUC, the prohibition on calling malloc, if it is still a concern,
> applies only when loading the dump, not while writing it.

That's my understanding as well.

> My main concern is the possibility of a partly-written dump file,
> since we no longer turn "!UMPEDGNUEMACS" into "DUMPEDGNUEMACS" after
> the dump. Maybe it would make sense to restore that feature?

Wouldn't hurt, though I'd make it low priority.

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Daniel Colascione]

On 3/3/21 12:51 AM, Eli Zaretskii wrote:

>> From: Pip Cet <pipcet@gmail.com>
>> Date: Tue, 2 Mar 2021 20:45:04 +0000
>>
>> On Tue, Mar 2, 2021 at 8:35 PM Pip Cet <pipcet@gmail.com> wrote:
>>> I've looked into the problem, and it seems easy to solve and worth it
>>> in terms of debuggability and performance.
>> Very rough benchmarks, but this seems to be clearly worth it:
>>
>> Performance:
>> With patch:
>> real    0m3.861s
>> user    0m3.776s
>> sys    0m0.085s
>>
>> Without patch:
>> real    0m7.001s
>> user    0m4.476s
>> sys    0m2.511s
>>
>> Number of syscalls:
>> With patch: 415442
>> Without patch: 2028307
>>
>>> Patch will be attached once this has a bug number.
>> And here's the patch. Testing would be very appreciated.
>>
>> I'm unsure about the precise usage of dump_off vs ptrdiff_t here; I
>> don't think it matters, but suggestions, nitpicks, and comments, on
>> this or any other aspect, would be very appreciated.
>>  From 92ee138852b34ede2f43dd7f93f310fc746bb3bf Mon Sep 17 00:00:00 2001
>> From: Pip Cet <pipcet@gmail.com>
>> Date: Tue, 2 Mar 2021 20:38:23 +0000
>> Subject: [PATCH] Prepare pdumper dump file in memory, write it in one go
>>   (Bug#46881)
>>
>> * src/pdumper.c (struct dump_context): Add buf, buf_size, max_offset fields.
>> (grow_buffer): New function.
>> (dump_write): Use memcpy, not an actual emacs_write.
>> (dump_seek): Keep track of maximum seen offset.
>> (Fdump_emacs_portable): Write out the file contents when done.
>> ---
>>   src/pdumper.c | 20 ++++++++++++++++++--
>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/pdumper.c b/src/pdumper.c
>> index 337742fda4ade..62ddad8ee5e34 100644
>> --- a/src/pdumper.c
>> +++ b/src/pdumper.c
>> @@ -473,6 +473,10 @@ dump_fingerprint (char const *label,
>>   {
>>     /* Header we'll write to the dump file when done.  */
>>     struct dump_header header;
>> +  /* Data that will be written to the dump file.  */
>> +  void *buf;
>> +  ptrdiff_t buf_size;
>> +  ptrdiff_t max_offset;
>>   
>>     Lisp_Object old_purify_flag;
>>     Lisp_Object old_post_gc_hook;
>> @@ -581,6 +585,13 @@ dump_fingerprint (char const *label,
>>   \f
>>   /* Dump file creation */
>>   
>> +static void dump_grow_buffer (struct dump_context *ctx)
>> +{
>> +  ctx->buf = xrealloc (ctx->buf, ctx->buf_size = (ctx->buf_size ?
>> +						  (ctx->buf_size * 2)
>> +						  : 1024 * 1024));
>> +}
>> +
>>   static dump_off dump_object (struct dump_context *ctx, Lisp_Object object);
>>   static dump_off dump_object_for_offset (struct dump_context *ctx,
>>   					Lisp_Object object);
>> @@ -747,8 +758,9 @@ dump_write (struct dump_context *ctx, const void *buf, dump_off nbyte)
>>     eassert (nbyte == 0 || buf != NULL);
>>     eassert (ctx->obj_offset == 0);
>>     eassert (ctx->flags.dump_object_contents);
>> -  if (emacs_write (ctx->fd, buf, nbyte) < nbyte)
>> -    report_file_error ("Could not write to dump file", ctx->dump_filename);
>> +  while (ctx->offset + nbyte > ctx->buf_size)
>> +    dump_grow_buffer (ctx);
>> +  memcpy ((char *)ctx->buf + ctx->offset, buf, nbyte);
>>     ctx->offset += nbyte;
>>   }
>>   
>> @@ -828,6 +840,8 @@ dump_tailq_pop (struct dump_tailq *tailq)
>>   static void
>>   dump_seek (struct dump_context *ctx, dump_off offset)
>>   {
>> +  if (ctx->max_offset < ctx->offset)
>> +    ctx->max_offset = ctx->offset;
>>     eassert (ctx->obj_offset == 0);
>>     if (lseek (ctx->fd, offset, SEEK_SET) < 0)
>>       report_file_error ("Setting file position",
>> @@ -4159,6 +4173,8 @@ DEFUN ("dump-emacs-portable",
>>     ctx->header.magic[0] = dump_magic[0];
>>     dump_seek (ctx, 0);
>>     dump_write (ctx, &ctx->header, sizeof (ctx->header));
>> +  if (emacs_write (ctx->fd, ctx->buf, ctx->max_offset) < ctx->max_offset)
>> +    report_file_error ("Could not write to dump file", ctx->dump_filename);
>>   
>>     dump_off
>>       header_bytes = header_end - header_start,
>> -- 
>> 2.30.1
> Thanks.
>
> Daniel, Paul: any comments?  In particular, is it safe to allocate
> large amounts of memory off the heap while dumping?  A couple of
> places in pdumper.c says some parts of code should call malloc.

It looks fine, but wouldn't dumping to a FILE* (with internal buffering) 
do the same basic thing in a simpler way? There aren't any particular 
constraints on the environment _during_ the dump: we even make new lisp 
objects. It's when loading the dump, early in initialization, that you 
have to be careful.

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Pip Cet]

On Thu, Mar 4, 2021 at 10:26 PM Daniel Colascione <dancol@dancol.org> wrote:
> On 3/3/21 12:51 AM, Eli Zaretskii wrote:
> > Daniel, Paul: any comments?  In particular, is it safe to allocate
> > large amounts of memory off the heap while dumping?  A couple of
> > places in pdumper.c says some parts of code should call malloc.
>
> It looks fine, but wouldn't dumping to a FILE* (with internal buffering)
> do the same basic thing in a simpler way?

I initially set out to do that, but decided against it. We don't just
write sequentially (when FILE I/O helps, a little), we also have the
seek-and-fixup phase, and it didn't seem any simpler at that point..

> There aren't any particular
> constraints on the environment _during_ the dump: we even make new lisp
> objects. It's when loading the dump, early in initialization, that you
> have to be careful.

Thanks!

Pip

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 5 Mar 2021 02:30:13 +0000
> Cc: Eli Zaretskii <eliz@gnu.org>, Paul Eggert <eggert@cs.ucla.edu>, 46881@debbugs.gnu.org
> 
> > It looks fine, but wouldn't dumping to a FILE* (with internal buffering)
> > do the same basic thing in a simpler way?
> 
> I initially set out to do that, but decided against it. We don't just
> write sequentially (when FILE I/O helps, a little), we also have the
> seek-and-fixup phase, and it didn't seem any simpler at that point..

I'm not sure I understand: what's wrong with fseek?

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Pip Cet]

On Fri, Mar 5, 2021 at 7:19 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Fri, 5 Mar 2021 02:30:13 +0000
> > Cc: Eli Zaretskii <eliz@gnu.org>, Paul Eggert <eggert@cs.ucla.edu>, 46881@debbugs.gnu.org
> >
> > > It looks fine, but wouldn't dumping to a FILE* (with internal buffering)
> > > do the same basic thing in a simpler way?
> >
> > I initially set out to do that, but decided against it. We don't just
> > write sequentially (when FILE I/O helps, a little), we also have the
> > seek-and-fixup phase, and it didn't seem any simpler at that point..
>
> I'm not sure I understand: what's wrong with fseek?

Nothing, assuming you're fine with the current performance. Many libcs
aren't going to be smart enough to avoid I/O when you fseek through a
"large" file and write a word here and there, and my suspicion is that
would include glibc.

Also, we're not currently using fseek-and-write anywhere in Emacs.

We're talking about a file which Emacs is going to have to keep in
memory anyway, when reading the dump. The only case in which there
might be a problem is if the build machine has significantly less
available memory than the machine we intend to run on, and I just
don't think that's going to happen.

Pip

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 5 Mar 2021 07:38:27 +0000
> Cc: Daniel Colascione <dancol@dancol.org>, eggert@cs.ucla.edu, 46881@debbugs.gnu.org
> 
> > I'm not sure I understand: what's wrong with fseek?
> 
> Nothing, assuming you're fine with the current performance. Many libcs
> aren't going to be smart enough to avoid I/O when you fseek through a
> "large" file and write a word here and there, and my suspicion is that
> would include glibc.

Could we benchmark the two implementations instead of acting on
suspicions?

In general, I'd prefer not to reinvent the wheel, and trust modern
libc's that they are efficient enough in handling buffered streams,
unless we have hard evidence to the contrary.  If nothing else, it
would prevent people asking, like Daniel did, why didn't we use stdio
in the first place.

> Also, we're not currently using fseek-and-write anywhere in Emacs.

I don't see why this would be important.  Since we open the file in
binary mode, fseek should work correctly even on non-Posix systems.
Am I missing something?

> We're talking about a file which Emacs is going to have to keep in
> memory anyway, when reading the dump. The only case in which there
> might be a problem is if the build machine has significantly less
> available memory than the machine we intend to run on, and I just
> don't think that's going to happen.

You are thinking about memory consumption, while I am thinking how to
avoid implementing our own private buffered streams.

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Pip Cet]

On Fri, Mar 5, 2021 at 7:55 AM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Fri, 5 Mar 2021 07:38:27 +0000
> > Cc: Daniel Colascione <dancol@dancol.org>, eggert@cs.ucla.edu, 46881@debbugs.gnu.org
> >
> > > I'm not sure I understand: what's wrong with fseek?
> >
> > Nothing, assuming you're fine with the current performance. Many libcs
> > aren't going to be smart enough to avoid I/O when you fseek through a
> > "large" file and write a word here and there, and my suspicion is that
> > would include glibc.
>
> Could we benchmark the two implementations instead of acting on
> suspicions?

Sure.

My patch:

real    0m1.988s
user    0m1.916s
sys    0m0.073s

fwrite-based patch:

real    0m3.576s
user    0m2.571s
sys    0m1.006s

This is as I expected: glibc just isn't doing a very good job for this
buffered stream.

> In general, I'd prefer not to reinvent the wheel, and trust modern
> libc's that they are efficient enough in handling buffered streams,
> unless we have hard evidence to the contrary.

We do, now.

> If nothing else, it
> would prevent people asking, like Daniel did, why didn't we use stdio
> in the first place.

I think it's a very good question (in fact, the brach I'm working on
is called pdumper-fwrite because I decided only after creating it that
all the seeking would hurt performance too much). I'll try including a
comment explaining why.

> > Also, we're not currently using fseek-and-write anywhere in Emacs.
>
> I don't see why this would be important.

Because the stream returned by emacs_fopen might not be generally seekable?

> Since we open the file in
> binary mode, fseek should work correctly even on non-Posix systems.

I guess I should have used emacs_fopen :-)

> > We're talking about a file which Emacs is going to have to keep in
> > memory anyway, when reading the dump. The only case in which there
> > might be a problem is if the build machine has significantly less
> > available memory than the machine we intend to run on, and I just
> > don't think that's going to happen.
>
> You are thinking about memory consumption, while I am thinking how to
> avoid implementing our own private buffered streams.

By preparing the data in memory and writing it in one go, which
doesn't require any of the major complications of implementing
buffered streams.

Pip

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Andrea Corallo via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pip Cet <pipcet@gmail.com> writes:

[...]

> By preparing the data in memory and writing it in one go, which
> doesn't require any of the major complications of implementing
> buffered streams.

Preparing data in memory might also be seen as a small step in the
direction of having pdumper as a generic de/serializer.  IMO would be
helpful for a number of tasks (native compiler included).

  Andrea

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 5 Mar 2021 09:54:32 +0000
> Cc: Daniel Colascione <dancol@dancol.org>, eggert@cs.ucla.edu, 46881@debbugs.gnu.org
> 
> My patch:
> 
> real    0m1.988s
> user    0m1.916s
> sys    0m0.073s
> 
> fwrite-based patch:
> 
> real    0m3.576s
> user    0m2.571s
> sys    0m1.006s

30% slowdown and 1.5 sec absolute time difference doesn't sound bad
enough to me to justify a homemade solution.  I say let's go with
stdio.

> > > Also, we're not currently using fseek-and-write anywhere in Emacs.
> >
> > I don't see why this would be important.
> 
> Because the stream returned by emacs_fopen might not be generally seekable?

I don't see how that could happen.

> > Since we open the file in
> > binary mode, fseek should work correctly even on non-Posix systems.
> 
> I guess I should have used emacs_fopen :-)

Yes, of course.  Especially as with fopen there are problems with
non-ASCII file names on MS-Windows.

> By preparing the data in memory and writing it in one go, which
> doesn't require any of the major complications of implementing
> buffered streams.

There are no complications I can see, not in our sources.  (And you
don't actually write it in one go anyway, see emacs_full_write.)

So let's go with the stdio solution, please.

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> 30% slowdown and 1.5 sec absolute time difference doesn't sound bad
> enough to me to justify a homemade solution.  I say let's go with
> stdio.

Seems significant to me -- we're building Emacs a lot, and this bit
can't be parallelised.  And the savings in electricity alone should make
us go for the most efficient solution.

There doesn't seem to be any significant drawbacks to doing it the
efficient way, either.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Pip Cet <pipcet@gmail.com>,  46881@debbugs.gnu.org,  eggert@cs.ucla.edu
> Date: Fri, 05 Mar 2021 13:49:42 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > 30% slowdown and 1.5 sec absolute time difference doesn't sound bad
> > enough to me to justify a homemade solution.  I say let's go with
> > stdio.
> 
> Seems significant to me -- we're building Emacs a lot, and this bit
> can't be parallelised.  And the savings in electricity alone should make
> us go for the most efficient solution.
> 
> There doesn't seem to be any significant drawbacks to doing it the
> efficient way, either.

<Shrug> Fine, let's do it that way, then.

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Pip Cet]

On Fri, Mar 5, 2021 at 12:07 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Fri, 5 Mar 2021 09:54:32 +0000
> > Cc: Daniel Colascione <dancol@dancol.org>, eggert@cs.ucla.edu, 46881@debbugs.gnu.org
> >
> > My patch:
> >
> > real    0m1.988s
> > user    0m1.916s
> > sys    0m0.073s
> >
> > fwrite-based patch:
> >
> > real    0m3.576s
> > user    0m2.571s
> > sys    0m1.006s
>
> 30% slowdown and 1.5 sec absolute time difference doesn't sound bad
> enough to me to

It's a 30% slowdown of the entire dump process, including the
CPU-intensive part which loads Emacs. I think you get a better idea of
the performance difference from the "sys" numbers above.

And the absolute time difference is more than that, because Emacs is
dumped twice during each build; the first dump file is about 2.5 times
the size of the ultimate dump file, so my guess (as I said before,
unfortunately Intel decided to make this system not have a predictable
CPU clock, so I can't really run good benchmarks) is we're talking
about 4.5 seconds here.

> justify a homemade solution.

"Create a buffer in memory and do all the IO at once" is such an old
solution that even the GNU Coding Standards explicitly recommend it
(albeit for input files):

You could keep the entire input file in memory and scan it there
instead of using stdio

>I say let's go with stdio.

Maybe setbuffer(3) could help us here? I could run some benchmarks for
that if the idea isn't out of the question.

> > > > Also, we're not currently using fseek-and-write anywhere in Emacs.
> > >
> > > I don't see why this would be important.
> >
> > Because the stream returned by emacs_fopen might not be generally seekable?
>
> I don't see how that could happen.

It has, to me, but I'm willing to accept I did some inadvisable things first.

> > By preparing the data in memory and writing it in one go, which
> > doesn't require any of the major complications of implementing
> > buffered streams.
>
> There are no complications I can see, not in our sources.  (And you
> don't actually write it in one go anyway, see emacs_full_write.)

Er, precisely. I was the one saying there are no complications, so we
shouldn't let the idea of "implementing our own buffered streams"
scare us, because that is a complicated project but it's also not what
we are doing.

> So let's go with the stdio solution, please.

Should I add a sync after every seek to make absolutely certain,
rather than merely likely, this will destroy someone's flash chip one
day?

Pip

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Pip Cet]

On Fri, Mar 5, 2021 at 1:16 PM Pip Cet <pipcet@gmail.com> wrote:
> >I say let's go with stdio.
>
> Maybe setbuffer(3) could help us here? I could run some benchmarks for
> that if the idea isn't out of the question.

Actually, calling setbuffer() with a large buffer will make glibc
reread the entire file on every fseek(), rendering the dump so slow I
gave up and interrupted it.

However, there's open_memstream: that would have the added advantage
of actually making glibc write out the entire file in one go, so it
seems to shave a few extra milliseconds off the build.

(However however, glibc's memstreams are somewhat broken. I'll file a
bug report if there isn't one already...)

Still, that way we could use stdio today, leave the buffering to
glibc, and hopefully be able to switch trivially to a "open this file
but keep it in memory" combined memstream/FILE* one day.

Pip

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Daniel Colascione]

On 3/5/21 9:02 AM, Pip Cet wrote:

> On Fri, Mar 5, 2021 at 1:16 PM Pip Cet <pipcet@gmail.com> wrote:
>>> I say let's go with stdio.
>> Maybe setbuffer(3) could help us here? I could run some benchmarks for
>> that if the idea isn't out of the question.
> Actually, calling setbuffer() with a large buffer will make glibc
> reread the entire file on every fseek(), rendering the dump so slow I
> gave up and interrupted it.
>
> However, there's open_memstream: that would have the added advantage
> of actually making glibc write out the entire file in one go, so it
> seems to shave a few extra milliseconds off the build.
>
> (However however, glibc's memstreams are somewhat broken. I'll file a
> bug report if there isn't one already...)
>
> Still, that way we could use stdio today, leave the buffering to
> glibc, and hopefully be able to switch trivially to a "open this file
> but keep it in memory" combined memstream/FILE* one day.

You could also use fopencookie to make your own stdio stream that does 
the right thing while still using the stdio abstraction in the part of 
the code actually doing the writing.

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Fri, 5 Mar 2021 14:02:33 +0000
> Cc: Daniel Colascione <dancol@dancol.org>, eggert@cs.ucla.edu, 46881@debbugs.gnu.org
> 
> However, there's open_memstream

That's glibc-only, AFAIK.  Not portable enough for us.

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Pip Cet]

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

On Fri, Mar 5, 2021 at 2:56 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Fri, 5 Mar 2021 14:02:33 +0000
> > Cc: Daniel Colascione <dancol@dancol.org>, eggert@cs.ucla.edu, 46881@debbugs.gnu.org
> >
> > However, there's open_memstream
>
> That's glibc-only, AFAIK.  Not portable enough for us.

POSIX.1-2008. Not portable enough to require, certainly, but portable
enough to use?

Pip

[-- Attachment #2: 0001-Use-stdio-memstreams-if-available-for-pdumper-Bug-46.patch --]
[-- Type: text/x-patch, Size: 4798 bytes --]

From b600c444e046d3888f56839dbe70d6d209a6ba29 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Tue, 2 Mar 2021 20:38:23 +0000
Subject: [PATCH] Use stdio, memstreams if available for pdumper (Bug#46881)

* configure.ac: Check for open_memstream.
* src/pdumper.c (dump_unwind_cleanup): Handle memstreams.
(dump_seek): Record maximum offset for memstreams. Use stdio.
(struct dump_context): Add buffer information for memstreams.
(Fdump_emacs_portable): Use memstreams if available.
(dump_write): Use stdio.
(Fdump_emacs_portable): Use memstreams if available.
---
 configure.ac  |  1 +
 src/pdumper.c | 64 ++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 51 insertions(+), 14 deletions(-)

diff --git a/configure.ac b/configure.ac
index 11a06a39bee3f..4225aea9635ce 100644
--- a/configure.ac
+++ b/configure.ac
@@ -4554,6 +4554,7 @@ AC_DEFUN
 dnl AC_CHECK_FUNCS_ONCE wouldn’t be right for snprintf, which needs
 dnl the current CFLAGS etc.
 AC_CHECK_FUNCS(snprintf)
+AC_CHECK_FUNCS(open_memstream)
 
 dnl Check for glib.  This differs from other library checks in that
 dnl Emacs need not link to glib unless some other library is already
diff --git a/src/pdumper.c b/src/pdumper.c
index 337742fda4ade..a524f87300b51 100644
--- a/src/pdumper.c
+++ b/src/pdumper.c
@@ -482,8 +482,14 @@ dump_fingerprint (char const *label,
   bool blocked_ralloc;
 #endif
 
-  /* File descriptor for dumpfile; < 0 if closed.  */
-  int fd;
+  /* File handle for dumpfile; NULL if closed.  */
+  FILE *file;
+#ifdef HAVE_OPEN_MEMSTREAM
+  /* buf is non-NULL if file referred to a memstream and has been closed. */
+  char *buf;
+  size_t buf_size;
+  size_t max_offset;
+#endif
   /* Name of dump file --- used for error reporting.  */
   Lisp_Object dump_filename;
   /* Current offset in dump file.  */
@@ -747,9 +753,16 @@ dump_write (struct dump_context *ctx, const void *buf, dump_off nbyte)
   eassert (nbyte == 0 || buf != NULL);
   eassert (ctx->obj_offset == 0);
   eassert (ctx->flags.dump_object_contents);
-  if (emacs_write (ctx->fd, buf, nbyte) < nbyte)
-    report_file_error ("Could not write to dump file", ctx->dump_filename);
-  ctx->offset += nbyte;
+  while (nbyte)
+    {
+      size_t res = fwrite (buf, 1, nbyte, ctx->file);
+      if (res == 0)
+	report_file_error ("Could not write to dump file",
+			   ctx->dump_filename);
+      buf = (char *)buf + res;
+      ctx->offset += res;
+      nbyte -= res;
+    }
 }
 
 static Lisp_Object
@@ -828,10 +841,13 @@ dump_tailq_pop (struct dump_tailq *tailq)
 static void
 dump_seek (struct dump_context *ctx, dump_off offset)
 {
+#ifdef HAVE_OPEN_MEMSTREAM
+  if (ctx->offset > ctx->max_offset)
+    ctx->max_offset = ctx->offset;
+#endif
   eassert (ctx->obj_offset == 0);
-  if (lseek (ctx->fd, offset, SEEK_SET) < 0)
-    report_file_error ("Setting file position",
-                       ctx->dump_filename);
+  if (fseek (ctx->file, offset, SEEK_SET) != 0)
+    report_file_error ("Setting file position", ctx->dump_filename);
   ctx->offset = offset;
 }
 
@@ -3513,8 +3529,25 @@ dump_drain_user_remembered_data_cold (struct dump_context *ctx)
 dump_unwind_cleanup (void *data)
 {
   struct dump_context *ctx = data;
-  if (ctx->fd >= 0)
-    emacs_close (ctx->fd);
+#ifdef HAVE_OPEN_MEMSTREAM
+  if (ctx->file)
+    {
+      fseek (ctx->file, ctx->max_offset, SEEK_SET);
+      fclose (ctx->file);
+    }
+  if (ctx->buf)
+    {
+      eassert (ctx->max_offset <= ctx->buf_size);
+      ctx->file = emacs_fopen (SSDATA (ctx->dump_filename), "w");
+      if (ctx->file == NULL)
+	report_file_error ("Could not open dump file",
+			   ctx->dump_filename);
+      ctx->offset = 0;
+      dump_write (ctx, ctx->buf, ctx->max_offset);
+    }
+#endif
+  if (ctx->file)
+    fclose (ctx->file);
 #ifdef REL_ALLOC
   if (ctx->blocked_ralloc)
     r_alloc_inhibit_buffer_relocation (0);
@@ -3952,7 +3985,7 @@ DEFUN ("dump-emacs-portable",
 
   struct dump_context ctx_buf = {0};
   struct dump_context *ctx = &ctx_buf;
-  ctx->fd = -1;
+  ctx->file = NULL;
 
   ctx->objects_dumped = make_eq_hash_table ();
   dump_queue_init (&ctx->dump_queue);
@@ -4012,9 +4045,12 @@ DEFUN ("dump-emacs-portable",
   ctx->old_process_environment = Vprocess_environment;
   Vprocess_environment = Qnil;
 
-  ctx->fd = emacs_open (SSDATA (filename),
-                        O_RDWR | O_TRUNC | O_CREAT, 0666);
-  if (ctx->fd < 0)
+#ifdef HAVE_OPEN_MEMSTREAM
+  ctx->file = open_memstream (&ctx->buf, &ctx->buf_size);
+#endif
+  if (ctx->file == NULL)
+    ctx->file = emacs_fopen (SSDATA (filename), "w+");
+  if (ctx->file == NULL)
     report_file_error ("Opening dump output", filename);
   verify (sizeof (ctx->header.magic) == sizeof (dump_magic));
   memcpy (&ctx->header.magic, dump_magic, sizeof (dump_magic));
-- 
2.30.1

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Andreas Schwab]

On Mär 05 2021, Pip Cet wrote:

> We're talking about a file which Emacs is going to have to keep in
> memory anyway, when reading the dump.

While reading the dump, you only have the data once, and you don't have
to realloc the whole data.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Pip Cet]

On Fri, Mar 5, 2021 at 9:35 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> On Mär 05 2021, Pip Cet wrote:
>
> > We're talking about a file which Emacs is going to have to keep in
> > memory anyway, when reading the dump.
>
> While reading the dump, you only have the data once, and you don't have
> to realloc the whole data.

Correct. I think a build memory usage of 28 MB for a 10 MB dump file
is something we can live with...

Pip

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Mattias Engdegård]

Any reason not to apply the patch from https://debbugs.gnu.org/cgi/bugreport.cgi?bug=46881#20 right away? I've been using it locally for quite some time with very good results.

There seems to be a consensus for it, and while there may be even better solutions, having this in place now won't hurt.

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Daniel Colascione]

On 6/15/21 2:25 AM, Mattias Engdegård wrote:

> Any reason not to apply the patch from https://debbugs.gnu.org/cgi/bugreport.cgi?bug=46881#20 right away? I've been using it locally for quite some time with very good results.
>
> There seems to be a consensus for it, and while there may be even better solutions, having this in place now won't hurt.
I thought we had consensus for an fwrite based approach?

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Eli Zaretskii]

> From: Daniel Colascione <dancol@dancol.org>
> Date: Tue, 15 Jun 2021 05:58:26 -0700
> 
> On 6/15/21 2:25 AM, Mattias Engdegård wrote:
> 
> > Any reason not to apply the patch from https://debbugs.gnu.org/cgi/bugreport.cgi?bug=46881#20 right away? I've been using it locally for quite some time with very good results.
> >
> > There seems to be a consensus for it, and while there may be even better solutions, having this in place now won't hurt.
> I thought we had consensus for an fwrite based approach?

That's what I preferred (still do), but Lars thought that the slightly
faster version without stdio was preferable.

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> That's what I preferred (still do), but Lars thought that the slightly
> faster version without stdio was preferable.

Well, it's 30% faster in a single-threaded phase of the compilation, so
it seems like a significant improvement to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Daniel Colascione]

On June 15, 2021 6:18:11 AM Lars Ingebrigtsen <larsi@gnus.org> wrote:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> That's what I preferred (still do), but Lars thought that the slightly
>> faster version without stdio was preferable.
>
> Well, it's 30% faster in a single-threaded phase of the compilation, so
> it seems like a significant improvement to me.
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>   bloggy blog: http://lars.ingebrigtsen.n

The in-memory version is 30% faster than the FILE* version or 30% faster 
than the current write () version?

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Eli Zaretskii]

> From: Daniel Colascione <dancol@dancol.org>
> CC: <mattiase@acm.org>, <pipcet@gmail.com>, <46881@debbugs.gnu.org>, <eggert@cs.ucla.edu>
> Date: Tue, 15 Jun 2021 06:25:50 -0700
> 
> >> That's what I preferred (still do), but Lars thought that the slightly
> >> faster version without stdio was preferable.
> >
> > Well, it's 30% faster in a single-threaded phase of the compilation, so
> > it seems like a significant improvement to me.
> >
> > --
> > (domestic pets only, the antidote for overdose, milk.)
> >   bloggy blog: http://lars.ingebrigtsen.n
> 
> The in-memory version is 30% faster than the FILE* version or 30% faster 
> than the current write () version?

The former, see

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=46881#56

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Mattias Engdegård]

15 juni 2021 kl. 15.17 skrev Lars Ingebrigtsen <larsi@gnus.org>:

> Well, it's 30% faster in a single-threaded phase of the compilation, so
> it seems like a significant improvement to me.

The patch seems to be good in all respects -- simple, low risk, better performance, no compatibility trouble. And nothing prevents us from replacing it with something better later on, should the need arise.

All set then?

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Daniel Colascione]

On 6/15/21 8:32 AM, Mattias Engdegård wrote:
> 15 juni 2021 kl. 15.17 skrev Lars Ingebrigtsen <larsi@gnus.org>:
>> Well, it's 30% faster in a single-threaded phase of the compilation, so
>> it seems like a significant improvement to me.
> The patch seems to be good in all respects -- simple, low risk, better performance, no compatibility trouble. And nothing prevents us from replacing it with something better later on, should the need arise.
>
> All set then?
>
Okay. I'm convinced.

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Mattias Engdegård]

16 juni 2021 kl. 00.44 skrev Daniel Colascione <dancol@dancol.org>:

> Okay. I'm convinced.

All right, pushed, and closing this bug.
Thanks, Pip!

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> All right, pushed, and closing this bug.
> Thanks, Pip!

Great; thanks.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Pip Cet]

On Wed, Jun 16, 2021 at 8:00 AM Mattias Engdegård <mattiase@acm.org> wrote:
>
> 16 juni 2021 kl. 00.44 skrev Daniel Colascione <dancol@dancol.org>:
>
> > Okay. I'm convinced.
>
> All right, pushed, and closing this bug.
> Thanks, Pip!

Thank you for following up on this! Sorry Emacs kind of lost priority
for me right now, but I'll get back to it...

bug#46881: 28.0.50; pdumper dumping causes way too many syscalls

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Pip Cet [2021-03-02 20:33:42] wrote:
> Playing around with the WebAssembly port, I noticed that pdumper, in
> creating the dump file, makes way too many syscalls: it uses
> emacs_write(), not fwrite(), so these calls translate to actual
> syscalls and context switches. On immature systems (or in special
> circumstances like a device mounted synchronously),

Thanks for this patch.  For the little story, this inefficiency showed
up on one of my Thinkpads running GNU/Linux with a plain
old ext4 partition mounted in the most standard way (no synchronous
mount or other funny business):

https://serverfault.com/questions/996495/writes-throttled-to-500kb-s

The way this manifested itself is that after some uptime individual
writes to the SSD became very slow.  For most operations, this was
completely invisible, but it was quite noticeable during Emacs's dump
which sometimes took several minutes (while all the rest of the
compilation (including the "load" part of the dump)) progressed at
(apparently) usual speeds.


        Stefan