CPU and GC cost of bignums

2020-02-04 Thread Ludovic Courtès
Hello!

(If you’re in a hurry, there are good news at the bottom.)

I noticed that 3.0 (and also 2.2 actually) takes a long time to compile
Guix’ gnu/services/mail.scm, which is macro-heavy, producing lots of
top-level defines.

At -O2 (the default), we have:

--8<---cut here---start->8---
scheme@(guile-user)> ,pr (compile-file "gnu/services/mail.scm")
% cumulative   self 
time   seconds seconds  procedure
 13.79 19.16 14.46  language/cps/slot-allocation.scm:846:19
 11.05 11.58 11.58  language/cps/intmap.scm:396:0:intmap-ref
  7.56 12.63  7.92  anon #x10768e0
  6.61  7.70  6.92  ice-9/popen.scm:145:0:reap-pipes
  5.50182.23  5.76  language/cps/intset.scm:470:5:visit-branch
  4.65  4.87  4.87  system/vm/linker.scm:179:0:string-table-intern!
  4.07  5.04  4.26  ice-9/vlist.scm:534:0:vhash-assoc
  3.54  3.93  3.71  language/cps/intmap.scm:184:0:intmap-add!
  3.28  6.65  3.43  language/cps/intset.scm:270:2:adjoin
  2.70  2.82  2.82  language/cps/intset.scm:349:0:intset-ref
  1.80 34.84  1.88  language/cps/intmap.scm:247:2:adjoin
  1.80  5.93  1.88  language/cps/intset.scm:269:0:intset-add
  1.74 18.22  1.83  language/cps/intmap.scm:246:0:intmap-add
  1.22  3.27  1.27  language/cps/intset.scm:382:2:visit-node
  1.16  2.94  1.22  language/cps/intset.scm:551:2:union
  1.11  1.38  1.16  language/cps/intset.scm:204:0:intset-add!
  0.74   1281.59  0.78  language/cps/intset.scm:472:5:visit-branch

[...]

Sample count: 1892
Total time: 104.795540582 seconds (85.091574653 seconds in GC)
--8<---cut here---end--->8---

At -O1:

--8<---cut here---start->8---
scheme@(guile-user)> ,use(system base optimize)
scheme@(guile-user)> ,pr (compile-file "gnu/services/mail.scm" #:opts 
(optimizations-for-level 1))
% cumulative   self 
time   seconds seconds  procedure
 11.76129.78  7.60  language/cps/intset.scm:470:5:visit-branch
 10.77  6.96  6.96  language/cps/intmap.scm:396:0:intmap-ref
 10.43 11.69  6.74  language/cps/slot-allocation.scm:846:19
  8.99  7.39  5.81  ice-9/vlist.scm:534:0:vhash-assoc
  7.55  4.88  4.88  system/vm/linker.scm:179:0:string-table-intern!
  6.44  4.16  4.16  ice-9/popen.scm:145:0:reap-pipes
  4.22  2.80  2.72  language/cps/intmap.scm:184:0:intmap-add!
  1.89  1.86  1.22  language/cps/slot-allocation.scm:681:17
  1.89  1.43  1.22  ice-9/vlist.scm:539:0:vhash-assq
  1.78  1.51  1.15  language/cps/slot-allocation.scm:505:17
  1.22  1.36  0.79  language/cps/slot-allocation.scm:846:19
  1.22  1.08  0.79  language/cps/slot-allocation.scm:505:17

[...]

Sample count: 901
Total time: 64.602907835 seconds (55.87541493 seconds in GC)
--8<---cut here---end--->8---

language/cps/slot-allocation.scm:846:19 corresponds to:

(define (compute-live-slots* slots label live-vars)
  (intset-fold (lambda (var live)
 (match (get-slot slots var)
   (#f live)
   (slot (add-live-slot slot live;L846
   (intmap-ref live-vars label)
   0))

The GC times remain extremely high though, and it’s also coming from
‘compute-live-slots*’:

--8<---cut here---start->8---
scheme@(guile-user)> (gcprof (lambda () (compile-file "gnu/services/mail.scm" 
#:opts (optimizations-for-level 1
% cumulative   self 
time   seconds seconds  procedure
 58.14 34.56 34.56  language/cps/slot-allocation.scm:846:19
  8.01  4.76  4.76  language/cps/slot-allocation.scm:681:17
  8.01  4.76  4.76  language/cps/slot-allocation.scm:505:17
  6.98  4.15  4.15  language/cps/slot-allocation.scm:505:17
  6.46  3.84  3.84  language/cps/slot-allocation.scm:846:19
  1.29  0.77  0.77  anon #x23e88e0

[...]

Sample count: 387
Total time: 59.442422179 seconds (50.331193744 seconds in GC)
--8<---cut here---end--->8---

(I believe Guile commit 5675e46410c9a24b05ddf58cbe3b998a4c9cad7c and its
parent were made to optimize the -O1 case back in 2017¹.)

‘compute-live-slots*’ returns an integer and the allocation comes from
line 846, where we allocate a bignum, in this case a verybignum even.
And for each bignum, we register a finalizer, which itself takes space.

(Time passes…)

The patch below (also for 2.2) gives us better timing:

--8<---cut here---start->8---
scheme@(guile-user)> (gcprof (lambda () (compile-file "gnu/services/mail.scm" 
#:opts (optimizations-for-level 1
% cumulative   self 
time   seconds seconds  procedure
 18.75  2.49  2.49  anon #x6f58e0

[..

[PATCH, v2] Fix build on ia64

2020-02-04 Thread John Paul Adrian Glaubitz
  * libguile/continuations.c (capture_auxiliary_stack): Fix
logic in preprocessor code when checking for ia64 host;
fix dereferencing of ctx variable.
  * libguile/threads.h (struct scm_thread): Add missing member
SCM_STACKITEM *auxiliary_stack_base.
---
 libguile/continuations.c | 6 +++---
 libguile/threads.h   | 5 +
 2 files changed, 8 insertions(+), 3 deletions(-)

 v2:
 - Make declaration of SCM_STACKITEM *auxiliary_stack_base
   conditional, i.e. only for SCM_HAVE_AUXILIARY_STACK.

diff --git a/libguile/continuations.c b/libguile/continuations.c
index 67a47d38c..b8b6e1dca 100644
--- a/libguile/continuations.c
+++ b/libguile/continuations.c
@@ -143,7 +143,7 @@ static void
 capture_auxiliary_stack (scm_thread *thread, scm_t_contregs *continuation)
 {
 #if SCM_HAVE_AUXILIARY_STACK
-# if !(defined __ia64 or defined __ia64__)
+# if !defined __ia64 || !defined __ia64__
 # error missing auxiliary stack implementation for architecture
 # endif
   char *top;
@@ -155,9 +155,9 @@ capture_auxiliary_stack (scm_thread *thread, scm_t_contregs 
*continuation)
 #if defined __hpux
   __uc_get_ar_bsp (ctx, (uint64_t *) &top);
 #elif defined linux
-  top = (char *) ctx->uc_mcontext.sc_ar_bsp;
+  top = (char *) ctx.uc_mcontext.sc_ar_bsp;
 #elif defined __FreeBSD__
-  top = (char *)(ctx->uc_mcontext.mc_special.bspstore
+  top = (char *)(ctx.uc_mcontext.mc_special.bspstore
  + ctx->uc_mcontext.mc_special.ndirty);
 #else
 #error missing auxiliary stack implementation for ia64 on this OS
diff --git a/libguile/threads.h b/libguile/threads.h
index 337dc83a9..e6a60e96b 100644
--- a/libguile/threads.h
+++ b/libguile/threads.h
@@ -118,6 +118,11 @@ struct scm_thread {
   /* Stack base.  Used when checking for C stack overflow.  */
   SCM_STACKITEM *base;
 
+#if SCM_HAVE_AUXILIARY_STACK
+  /* Auxiliary stack base. */
+  SCM_STACKITEM *auxiliary_stack_base;
+#endif
+
   /* JIT state; NULL until this thread needs to JIT-compile something.  */
   struct scm_jit_state *jit_state;
 };
-- 
2.25.0




[PATCH] Fix build on ia64

2020-02-04 Thread John Paul Adrian Glaubitz
  * libguile/continuations.c (capture_auxiliary_stack): Fix
logic in preprocessor code when checking for ia64 host;
fix dereferencing of ctx variable.
  * libguile/threads.h (struct scm_thread): Add missing member
SCM_STACKITEM *auxiliary_stack_base.
---
 libguile/continuations.c | 6 +++---
 libguile/threads.h   | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/libguile/continuations.c b/libguile/continuations.c
index 67a47d38c..b8b6e1dca 100644
--- a/libguile/continuations.c
+++ b/libguile/continuations.c
@@ -143,7 +143,7 @@ static void
 capture_auxiliary_stack (scm_thread *thread, scm_t_contregs *continuation)
 {
 #if SCM_HAVE_AUXILIARY_STACK
-# if !(defined __ia64 or defined __ia64__)
+# if !defined __ia64 || !defined __ia64__
 # error missing auxiliary stack implementation for architecture
 # endif
   char *top;
@@ -155,9 +155,9 @@ capture_auxiliary_stack (scm_thread *thread, scm_t_contregs 
*continuation)
 #if defined __hpux
   __uc_get_ar_bsp (ctx, (uint64_t *) &top);
 #elif defined linux
-  top = (char *) ctx->uc_mcontext.sc_ar_bsp;
+  top = (char *) ctx.uc_mcontext.sc_ar_bsp;
 #elif defined __FreeBSD__
-  top = (char *)(ctx->uc_mcontext.mc_special.bspstore
+  top = (char *)(ctx.uc_mcontext.mc_special.bspstore
  + ctx->uc_mcontext.mc_special.ndirty);
 #else
 #error missing auxiliary stack implementation for ia64 on this OS
diff --git a/libguile/threads.h b/libguile/threads.h
index 337dc83a9..a5de6c7b2 100644
--- a/libguile/threads.h
+++ b/libguile/threads.h
@@ -118,6 +118,9 @@ struct scm_thread {
   /* Stack base.  Used when checking for C stack overflow.  */
   SCM_STACKITEM *base;
 
+  /* Auxiliary stack base. */
+  SCM_STACKITEM *auxiliary_stack_base;
+
   /* JIT state; NULL until this thread needs to JIT-compile something.  */
   struct scm_jit_state *jit_state;
 };
-- 
2.25.0




[PATCH, v2] Fix build on platforms where the stack grows upwards

2020-02-04 Thread John Paul Adrian Glaubitz
 * libguile/continuations.c (scm_dynthrow): Fix missing mra
   parameter to grow_stack for SCM_STACK_GROWS_UP.
---
 libguile/continuations.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

 v2:
 - Improved grammar in commit message

diff --git a/libguile/continuations.c b/libguile/continuations.c
index 3f86c6bd4..67a47d38c 100644
--- a/libguile/continuations.c
+++ b/libguile/continuations.c
@@ -323,7 +323,7 @@ scm_dynthrow (SCM cont, uint8_t *mra)
 
 #if SCM_STACK_GROWS_UP
   if (dst + continuation->num_stack_items >= &stack_top_element)
-grow_stack (cont);
+grow_stack (cont, mra);
 #else
   dst -= continuation->num_stack_items;
   if (dst <= &stack_top_element)
-- 
2.25.0




Re: Removal of hppa support

2020-02-04 Thread John Paul Adrian Glaubitz
Hi!

On 2/4/20 12:23 PM, Andy Wingo wrote:
>> On 1/27/20 4:46 PM, Andy Wingo wrote:
>>> William is correct.  HPPA support is not gone from Guile; and indeed
>>> it's good to hear from you :)  I wasn't sure there were any IA64 users
>>> remaining.
>>
>> It fails to build from source on Debian hppa, however:
> 
> Thanks for these reports and apologies for the breakages.  Looking
> forward to the patches.

Sure. Just sent the one for hppa.

>> I fixed the preprocessor conditional, but then I'm running into another
>> issue:
>>
>> continuations.c: In function 'capture_auxiliary_stack':
>> continuations.c:152:7: warning: implicit declaration of function 
>> 'getcontext' [-Wimplicit-function-declaration]
> 
> Probably a missing include for ucontext.h.

It's there:

#if SCM_HAVE_AUXILIARY_STACK
#include 
#endif

There are some more issues. I wills send a patch for ia64 in a bit.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913



Fix build of platforms where the stack grows upwards

2020-02-04 Thread John Paul Adrian Glaubitz
Hi!

In a20feea43, the mra of continuation was included in scm_dynthrow but
it was forgotten for SCM_STACK_GROWS_UP when calling grow_stack which
causes guile failing to build on Linux/hppa.

Thanks,
Adrian





[PATCH] Fix build of platforms where the stack grows upwards

2020-02-04 Thread John Paul Adrian Glaubitz
 * libguile/continuations.c (scm_dynthrow): Fix missing mra
   parameter to grow_stack for SCM_STACK_GROWS_UP.
---
 libguile/continuations.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libguile/continuations.c b/libguile/continuations.c
index 3f86c6bd4..67a47d38c 100644
--- a/libguile/continuations.c
+++ b/libguile/continuations.c
@@ -323,7 +323,7 @@ scm_dynthrow (SCM cont, uint8_t *mra)
 
 #if SCM_STACK_GROWS_UP
   if (dst + continuation->num_stack_items >= &stack_top_element)
-grow_stack (cont);
+grow_stack (cont, mra);
 #else
   dst -= continuation->num_stack_items;
   if (dst <= &stack_top_element)
-- 
2.25.0




Re: Removal of hppa support

2020-02-04 Thread Andy Wingo
Greets,

On Tue 04 Feb 2020 12:02, John Paul Adrian Glaubitz 
 writes:

> On 1/27/20 4:46 PM, Andy Wingo wrote:
>> William is correct.  HPPA support is not gone from Guile; and indeed
>> it's good to hear from you :)  I wasn't sure there were any IA64 users
>> remaining.
>
> It fails to build from source on Debian hppa, however:

Thanks for these reports and apologies for the breakages.  Looking
forward to the patches.

> I fixed the preprocessor conditional, but then I'm running into another
> issue:
>
> continuations.c: In function 'capture_auxiliary_stack':
> continuations.c:152:7: warning: implicit declaration of function 'getcontext' 
> [-Wimplicit-function-declaration]

Probably a missing include for ucontext.h.

>> If someone would like to write an IA64 backend for Lightening, I would
>> be happy to accept it :)  The beginnings of one are there in the git
>> history.
>
> Ok. I assume that applies to alpha, hppa, m68k, powerpc*, riscv*, sparc* as 
> well.

Yes indeed!

Andy



Re: Removal of hppa support

2020-02-04 Thread John Paul Adrian Glaubitz
Hi!

On 1/27/20 4:46 PM, Andy Wingo wrote:
> William is correct.  HPPA support is not gone from Guile; and indeed
> it's good to hear from you :)  I wasn't sure there were any IA64 users
> remaining.

It fails to build from source on Debian hppa, however:

> https://buildd.debian.org/status/fetch.php?pkg=guile-3.0&arch=hppa&ver=3.0.0%2B1-1&stamp=1580702308&raw=0

continuations.c: In function 'scm_dynthrow':
continuations.c:326:5: error: too few arguments to function 'grow_stack'
  326 | grow_stack (cont);
  | ^~
continuations.c:276:1: note: declared here
  276 | grow_stack (SCM cont, uint8_t *mra)
  | ^~

I'm currently working on a patch.

> Initially in Guile I planned to use GNU Lightning, in part because of
> its great platform support.  However it turned out to not be the right
> thing, and reluctantly I ended up doing something that was more like a
> rewrite than a refactor.  In that context I personally don't have the
> budget to write the IA64 backend.  So, Guile 3 still runs on IA64, just
> without JIT support.

It also fails on ia64 at the moment:

> https://buildd.debian.org/status/fetch.php?pkg=guile-3.0&arch=ia64&ver=3.0.0%2B1-1&stamp=1580702151&raw=0

continuations.c:146:23: error: missing binary operator before token "or"
  146 | # if !(defined __ia64 or defined __ia64__)
  |   ^~

I fixed the preprocessor conditional, but then I'm running into another
issue:

continuations.c: In function 'capture_auxiliary_stack':
continuations.c:152:7: warning: implicit declaration of function 'getcontext' 
[-Wimplicit-function-declaration]
  152 |   if (getcontext (&ctx) != 0)
  |   ^~
continuations.c:158:21: error: invalid type argument of '->' (have 'ucontext_t' 
{aka 'struct ucontext_t'})
  158 |   top = (char *) ctx->uc_mcontext.sc_ar_bsp;
  | ^~
continuations.c:167:26: error: 'scm_thread' {aka 'struct scm_thread'} has no 
member named 'auxiliary_stack_base'
  167 | top - (char *) thread->auxiliary_stack_base;
  |  ^~
continuations.c:171:48: error: 'scm_thread' {aka 'struct scm_thread'} has no 
member named 'auxiliary_stack_base'
  171 |   memcpy (continuation->auxiliary_stack, thread->auxiliary_stack_base,
  |^~
continuations.c: In function 'restore_auxiliary_stack':
continuations.c:180:17: error: 'scm_thread' {aka 'struct scm_thread'} has no 
member named 'auxiliary_stack_base'
  180 |   memcpy (thread->auxiliary_stack_base, continuation->auxiliary_stack,
  | ^~

Working on a patch here as well.

> If someone would like to write an IA64 backend for Lightening, I would
> be happy to accept it :)  The beginnings of one are there in the git
> history.

Ok. I assume that applies to alpha, hppa, m68k, powerpc*, riscv*, sparc* as 
well.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaub...@debian.org
`. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
  `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913