Re: Weak tables harmful to GC?

[Ludovic Courtès]

Heya!

Andy Wingo  skribis:

> On Tue 31 Oct 2017 00:13, l...@gnu.org (Ludovic Courtès) writes:
>
>> I built libguile with the patch (I haven’t yet taken the time to rebuild
>> all of Guile).  It works, but unfortunately it still shows quick growth
>> of the heap on this example ():
>
> Fixed in attached patches (on top of the other one).  This was a race
> between the periodic vacuum process, which should run after GC via a
> finalizer, and the mutator.  If the mutator had the weak table lock, the
> vacuum would never be run.  Of course in the test below, the mutator
> usually has the table lock, so we ended up often skipping the vacuum,
> which causes the table size to grow, which causes the active heap size
> to grow, which causes the bytes-allocated-before-GC to increase, and
> which ultimately is a vicious circle.
>
> In my tests this patch fixes the issue, though the level at which the
> heap stabilizes can vary slightly as there's a bit of nondeterministic
> concurrency as the mutator and the vacuum process still race a bit.
> Right now for me it stabilizes at about 6.2M of heap.

Yes.  I can confirm that it fixes this issue.

One of my benchmarks is to ‘read’ the 26M file that contains the
CPS-as-sexps representation of gnu/packages/python.scm, with
source-location recording on (thus with a big source property table).

Compared to 2.2.2, execution time is almost divided by two; heap size is
usually 540M and 620M, whereas it varies between 560M and 920M with
2.2.2:

--8<---cut here---start->8---
ludo@ribbon ~/src/guix$ guile profile-cps-read.scm
clock utime stime cutime cstime gctime
 8.23 10.00  0.06   0.00   0.00   5.17

;;; (#)
with Guile 2.2.2:
heap-size from 2.53 MiB to 780.03 MiB
ludo@ribbon ~/src/guix$ guile profile-cps-read.scm
clock utime stime cutime cstime gctime
 8.29  9.94  0.04   0.00   0.00   4.97

;;; (#)
with Guile 2.2.2:
heap-size from 2.53 MiB to 660.36 MiB
ludo@ribbon ~/src/guix$ guile profile-cps-read.scm
clock utime stime cutime cstime gctime
 8.11  9.33  0.05   0.00   0.00   3.90

;;; (#)
with Guile 2.2.2:
heap-size from 2.53 MiB to 562.21 MiB
ludo@ribbon ~/src/guix$ guile profile-cps-read.scm
clock utime stime cutime cstime gctime
 8.23  9.90  0.08   0.00   0.00   5.22

;;; (#)
with Guile 2.2.2:
heap-size from 2.53 MiB to 918.09 MiB
ludo@ribbon ~/src/guix$ /data/src/guile-2.1/meta/guile profile-cps-read.scm
clock utime stime cutime cstime gctime
 4.68  6.71  0.02   0.00   0.00   3.88

;;; (#)
with Guile 2.2.2.17-8069-dirty:
heap-size from 1.89 MiB to 540.54 MiB
ludo@ribbon ~/src/guix$ /data/src/guile-2.1/meta/guile profile-cps-read.scm
clock utime stime cutime cstime gctime
 4.73  6.77  0.05   0.00   0.00   4.03

;;; (#)
with Guile 2.2.2.17-8069-dirty:
heap-size from 1.89 MiB to 620.22 MiB
ludo@ribbon ~/src/guix$ /data/src/guile-2.1/meta/guile profile-cps-read.scm
clock utime stime cutime cstime gctime
 4.69  6.80  0.06   0.00   0.00   3.94

;;; (#)
with Guile 2.2.2.17-8069-dirty:
heap-size from 1.89 MiB to 573.36 MiB
ludo@ribbon ~/src/guix$ /data/src/guile-2.1/meta/guile profile-cps-read.scm
clock utime stime cutime cstime gctime
 4.67  6.81  0.06   0.00   0.00   4.05

;;; (#)
with Guile 2.2.2.17-8069-dirty:
heap-size from 1.89 MiB to 547.39 MiB
--8<---cut here---end--->8---

Another benchmark is the ‘emit-bytecode’ procedure for this CPS, without
optimizations turned off (like ‘%lightweight-optimizations’ in Guix.)

The heap size after the ‘emit-bytecode’ call is still at 1.3G (338M
before the call), about the same as with 2.2.2.  That’s not surprising
because I think memory consumption comes from the data structures used
at that compilation stage, as discussed before.

The wall-clock time is ~45s, whereas it’s ~54s with 2.2.2.

In other news, I’ve rebuilt 2.2.2 + these patches with Guix, and
everything went fine (Guile processes seem to peak at ~150M resident
when compiling).

So, all the lights are green on my side!

Thanks a whole lot for coming up with this solution!

Ludo’.

Re: Weak tables harmful to GC?

[Andy Wingo]

On Tue 31 Oct 2017 00:13, l...@gnu.org (Ludovic Courtès) writes:

> I built libguile with the patch (I haven’t yet taken the time to rebuild
> all of Guile).  It works, but unfortunately it still shows quick growth
> of the heap on this example ():

Fixed in attached patches (on top of the other one).  This was a race
between the periodic vacuum process, which should run after GC via a
finalizer, and the mutator.  If the mutator had the weak table lock, the
vacuum would never be run.  Of course in the test below, the mutator
usually has the table lock, so we ended up often skipping the vacuum,
which causes the table size to grow, which causes the active heap size
to grow, which causes the bytes-allocated-before-GC to increase, and
which ultimately is a vicious circle.

In my tests this patch fixes the issue, though the level at which the
heap stabilizes can vary slightly as there's a bit of nondeterministic
concurrency as the mutator and the vacuum process still race a bit.
Right now for me it stabilizes at about 6.2M of heap.

>>weak_key_gc_kind =
>>  GC_new_kind (GC_new_free_list (),
>> - GC_MAKE_PROC (GC_new_proc (mark_weak_key_table), 0),
>> + GC_MAKE_PROC (GC_new_proc (mark_weak_key_entry), 0),
>>   0, 0);
>
> I think we should avoid custom mark procedures and use a bitmap here, as
> recommended in the libgc headers (like ‘wcar_pair_descr’ in weaks.c in
> 2.0.)

Good idea; fixed.

Andy

>From 098c4171ef53791d97b5c675218f302efc7bcf26 Mon Sep 17 00:00:00 2001
From: Andy Wingo 
Date: Tue, 31 Oct 2017 09:10:55 +0100
Subject: [PATCH 2/3] Refactor weak table to use bitmaps for weak entries

---
 libguile/weak-table.c | 107 --
 1 file changed, 25 insertions(+), 82 deletions(-)

diff --git a/libguile/weak-table.c b/libguile/weak-table.c
index ff8a01fb0..fab98149f 100644
--- a/libguile/weak-table.c
+++ b/libguile/weak-table.c
@@ -25,7 +25,7 @@
 #include 
 
 #include "libguile/bdw-gc.h"
-#include 
+#include 
 
 #include "libguile/_scm.h"
 #include "libguile/hash.h"
@@ -152,70 +152,10 @@ typedef struct {
 
 
 
-/* The GC "kinds" for singly-weak tables.  */
-static int weak_key_gc_kind;
-static int weak_value_gc_kind;
-static int doubly_weak_gc_kind;
-
-static struct GC_ms_entry *
-mark_weak_key_entry (GC_word *addr, struct GC_ms_entry *mark_stack_ptr,
- struct GC_ms_entry *mark_stack_limit, GC_word env)
-{
-  scm_t_weak_entry *entry = (scm_t_weak_entry*) addr;
-
-  if (entry->next)
-mark_stack_ptr = GC_MARK_AND_PUSH ((GC_word*) entry->next,
-   mark_stack_ptr, mark_stack_limit,
-   NULL);
-
-  if (entry->hash && entry->key)
-{
-  SCM value = SCM_PACK (entry->value);
-  if (SCM_HEAP_OBJECT_P (value))
-mark_stack_ptr = GC_MARK_AND_PUSH ((GC_word*) SCM2PTR (value),
-   mark_stack_ptr, mark_stack_limit,
-   NULL);
-}
-
-  return mark_stack_ptr;
-}
-
-static struct GC_ms_entry *
-mark_weak_value_entry (GC_word *addr, struct GC_ms_entry *mark_stack_ptr,
-   struct GC_ms_entry *mark_stack_limit, GC_word env)
-{
-  scm_t_weak_entry *entry = (scm_t_weak_entry*) addr;
-
-  if (entry->next)
-mark_stack_ptr = GC_MARK_AND_PUSH ((GC_word*) entry->next,
-   mark_stack_ptr, mark_stack_limit,
-   NULL);
-
-  if (entry->hash && entry->value)
-{
-  SCM key = SCM_PACK (entry->key);
-  if (SCM_HEAP_OBJECT_P (key))
-mark_stack_ptr = GC_MARK_AND_PUSH ((GC_word*) SCM2PTR (key),
-   mark_stack_ptr, mark_stack_limit,
-   NULL);
-}
-
-  return mark_stack_ptr;
-}
-
-static struct GC_ms_entry *
-mark_doubly_weak_entry (GC_word *addr, struct GC_ms_entry *mark_stack_ptr,
-struct GC_ms_entry *mark_stack_limit, GC_word env)
-{
-  scm_t_weak_entry *entry = (scm_t_weak_entry*) addr;
-
-  if (entry->next)
-mark_stack_ptr = GC_MARK_AND_PUSH ((GC_word*) entry->next,
-   mark_stack_ptr, mark_stack_limit,
-   NULL);
-
-  return mark_stack_ptr;
-}
+/* GC descriptors for the various kinds of scm_t_weak_entry.  */
+static GC_descr weak_key_descr;
+static GC_descr weak_value_descr;
+static GC_descr doubly_weak_descr;
 
 static scm_t_weak_entry *
 allocate_entry (scm_t_weak_table_kind kind)
@@ -225,20 +165,18 @@ allocate_entry (scm_t_weak_table_kind kind)
   switch (kind)
 {
 case SCM_WEAK_TABLE_KIND_KEY:
-  ret = GC_generic_malloc (sizeof (*ret), weak_key_gc_kind);
+  ret = GC_malloc_explicitly_typed (sizeof (*ret), weak_key_descr);
   break;
 case SCM_WEAK_TABLE_KIND_VALUE:
-  ret = GC_generic_malloc 

Re: Weak tables harmful to GC?

[Ludovic Courtès]

Hi Andy,

Andy Wingo  skribis:

> As discussed on IRC, what do you think of this patch?  It preserves the
> thread-safety properties of weak tables and just adapts them to be
> bucket-and-chain tables.  Let me know how it works for you.

That was fast!  The code certainly looks nicer than the old entangled
weak hash table code, and it preserves thread-safety, so that’s great.

> If it works, we'll need to adapt weak sets as well.

Yes, though I think weak sets are less critical.

I built libguile with the patch (I haven’t yet taken the time to rebuild
all of Guile).  It works, but unfortunately it still shows quick growth
of the heap on this example ():

--8<---cut here---start->8---
(use-modules (ice-9 format))

(define (display-heap-size)
  (format #t "heap size: ~,2h MiB~%"
  (/ (assoc-ref (gc-stats) 'heap-size) (expt 2. 20

(define table
  (make-weak-key-hash-table))

(let loop ((i 0))
  (unless #f
(when (zero? (modulo i 10))
  (pk 'table table)
  (display-heap-size))
(hashq-set! table (make-list 10) (make-list 10))
(loop (1+ i
--8<---cut here---end--->8---

Could it me that some of the disappearing links are not getting
unregistered?

> From 6ec4642516eaabf7a63644463a7836eb3efbcd60 Mon Sep 17 00:00:00 2001
> From: Andy Wingo 
> Date: Mon, 30 Oct 2017 18:19:37 +0100
> Subject: [PATCH] Weak tables are now bucket-and-chain tables
>
> This change should make weak tables work better with libgc, as the weak
> components that need mark functions are smaller, so they don't overflow
> the mark queue.  Also this prevents the need to move disappearing
> links.
>
> * libguile/weak-table.c (scm_t_weak_entry): Change to be a hash table
>   chain entry.
>   (struct weak_entry_data, do_read_weak_entry, read_weak_entry): Read
>   out the key and value directly.
>   (GC_move_disappearing_link, move_disappearing_links, move_weak_entry):
>   Remove.
>   (scm_t_weak_table): Rename "entries" member to "buckets", and "size" to
>   "n_buckets".
>   (hash_to_index, entry_distance, rob_from_rich, give_to_poor): Remove.
>   (mark_weak_key_entry, mark_weak_value_entry): Mark a single link, and
>   the next link.
>   (mark_doubly_weak_entry): New kind.
>   (allocate_entry): Allocate a single entry.
>   (add_entry): New helper.
>   (resize_table): Reimplement more like normal hash tables.
>   (vacuum_weak_table): Adapt to new implementation.
>   (weak_table_ref, weak_table_put_x, weak_table_remove_x): Adapt.
>   (make_weak_table): Adapt.
>   (scm_weak_table_clear_x): Actually unregister the links to prevent a
>   memory leak.
>   (scm_c_weak_table_fold): Collect items in an alist, then fold outside
>   the lock.
>   (scm_weak_table_prehistory): Initialize doubly_weak_gc_kind.

[...]

> +mark_weak_key_entry (GC_word *addr, struct GC_ms_entry *mark_stack_ptr,
>   struct GC_ms_entry *mark_stack_limit, GC_word env)

[...]

>weak_key_gc_kind =
>  GC_new_kind (GC_new_free_list (),
> -  GC_MAKE_PROC (GC_new_proc (mark_weak_key_table), 0),
> +  GC_MAKE_PROC (GC_new_proc (mark_weak_key_entry), 0),
>0, 0);

I think we should avoid custom mark procedures and use a bitmap here, as
recommended in the libgc headers (like ‘wcar_pair_descr’ in weaks.c in
2.0.)

Other than that, it looks good on a cursory look.  We’ll have to do some
more testing afterwards to gain more confidence, like what Ricardo has
been doing.

Thanks a lot for your help!

Ludo’.

Re: Weak tables harmful to GC?

[Ludovic Courtès]

Hi,

Ricardo Wurmus  skribis:

> previously I wrote:
>
>> The “guile-awesome” package finished compiling (after about 46 minutes).
>> I’m now testing “guix pull” with a version of Guix that uses
>> “guile-awesome”.
>
> I’m sure I’m doing something wrong (see below for guesses).  Here’s what
> I get:
>
> ./pre-inst-env guix pull
> loading...   26.0% of 645 filesrandom seed for tests: 1509382171
> compiling... 18.9% of 645 filesIn thread:
> ERROR: In procedure return: return used outside of 'with-monad'Error while 
> printing exception.
> compiling... 54.7% of 645 files^C

The error above is the other bug you reported, not related (but just as
serious): .

> I modified build-self.scm to use the modified Guile:
>
> diff --git a/build-aux/build-self.scm b/build-aux/build-self.scm
> index ed8ff5f..9af6504 100644
> --- a/build-aux/build-self.scm
> +++ b/build-aux/build-self.scm
> @@ -126,7 +126,7 @@ running Guile."
>(package->derivation (cond-expand
>   (guile-2.2
>(canonical-package
> -   (specification->package "guile@2.2")))
> +   (specification->package "guile-awesome@2.2")))
>   (else
>(canonical-package
> (specification->package "guile@2.0"))
>
> I also confirmed that the Guile process that is spawned as “bin/guile
> --no-auto-compile /home/rwurmus/guix/scripts/guix pull” is indeed the
> modified Guile, but I noticed that it spawns yet another Guile process
> to load and compile Guix.
>
> I guess that comes from the daemon?  If that’s the case I can’t really
> test this on this big server, because the daemon is currently in use, so
> I can’t just reconfigure it to use the modified Guile.

Your patch above should have led to the use of “guile-awesome” to
compile Guix; I’m not sure why it didn’t.

> When compiling Guix from source with “make -j 32” using that version of
> Guile I got a segfault.

Oh?

Let’s put this on hold since Andy offers a different solution.

Thanks for testing!

Ludo’.

Re: Weak tables harmful to GC?

[Andy Wingo]

Hi!

As discussed on IRC, what do you think of this patch?  It preserves the
thread-safety properties of weak tables and just adapts them to be
bucket-and-chain tables.  Let me know how it works for you.  If it
works, we'll need to adapt weak sets as well.

Andy

>From 6ec4642516eaabf7a63644463a7836eb3efbcd60 Mon Sep 17 00:00:00 2001
From: Andy Wingo 
Date: Mon, 30 Oct 2017 18:19:37 +0100
Subject: [PATCH] Weak tables are now bucket-and-chain tables

This change should make weak tables work better with libgc, as the weak
components that need mark functions are smaller, so they don't overflow
the mark queue.  Also this prevents the need to move disappearing
links.

* libguile/weak-table.c (scm_t_weak_entry): Change to be a hash table
  chain entry.
  (struct weak_entry_data, do_read_weak_entry, read_weak_entry): Read
  out the key and value directly.
  (GC_move_disappearing_link, move_disappearing_links, move_weak_entry):
  Remove.
  (scm_t_weak_table): Rename "entries" member to "buckets", and "size" to
  "n_buckets".
  (hash_to_index, entry_distance, rob_from_rich, give_to_poor): Remove.
  (mark_weak_key_entry, mark_weak_value_entry): Mark a single link, and
  the next link.
  (mark_doubly_weak_entry): New kind.
  (allocate_entry): Allocate a single entry.
  (add_entry): New helper.
  (resize_table): Reimplement more like normal hash tables.
  (vacuum_weak_table): Adapt to new implementation.
  (weak_table_ref, weak_table_put_x, weak_table_remove_x): Adapt.
  (make_weak_table): Adapt.
  (scm_weak_table_clear_x): Actually unregister the links to prevent a
  memory leak.
  (scm_c_weak_table_fold): Collect items in an alist, then fold outside
  the lock.
  (scm_weak_table_prehistory): Initialize doubly_weak_gc_kind.
---
 libguile/weak-table.c | 723 +++---
 1 file changed, 212 insertions(+), 511 deletions(-)

diff --git a/libguile/weak-table.c b/libguile/weak-table.c
index 599c4cf0e..ff8a01fb0 100644
--- a/libguile/weak-table.c
+++ b/libguile/weak-table.c
@@ -1,4 +1,4 @@
-/* Copyright (C) 2011, 2012, 2013, 2014 Free Software Foundation, Inc.
+/* Copyright (C) 2011, 2012, 2013, 2014, 2017 Free Software Foundation, Inc.
  * 
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public License
@@ -44,83 +44,62 @@
data, but when you don't have space to store the data in the object.
For example, procedure properties are implemented with weak tables.
 
-   Weak tables are implemented using an open-addressed hash table.
-   Basically this means that there is an array of entries, and the item
-   is expected to be found the slot corresponding to its hash code,
-   modulo the length of the array.
-
-   Collisions are handled using linear probing with the Robin Hood
-   technique.  See Pedro Celis' paper, "Robin Hood Hashing":
-
- http://www.cs.uwaterloo.ca/research/tr/1986/CS-86-14.pdf
-
-   The vector of entries is allocated in such a way that the GC doesn't
-   trace the weak values.  For doubly-weak tables, this means that the
-   entries are allocated as an "atomic" piece of memory.  Key-weak and
-   value-weak tables use a special GC kind with a custom mark procedure.
-   When items are added weakly into table, a disappearing link is
-   registered to their locations.  If the referent is collected, then
-   that link will be zeroed out.
+   This is a normal bucket-and-chain hash table, except that the chain
+   entries are allocated in such a way that the GC doesn't trace the
+   weak values.  For doubly-weak tables, this means that the entries are
+   allocated as an "atomic" piece of memory.  Key-weak and value-weak
+   tables use a special GC kind with a custom mark procedure.  When
+   items are added weakly into table, a disappearing link is registered
+   to their locations.  If the referent is collected, then that link
+   will be zeroed out.
 
An entry in the table consists of the key and the value, together
-   with the hash code of the key.  We munge hash codes so that they are
-   never 0.  In this way we can detect removed entries (key of zero but
-   nonzero hash code), and can then reshuffle elements as needed to
-   maintain the robin hood ordering.
-
-   Compared to buckets-and-chains hash tables, open addressing has the
-   advantage that it is very cache-friendly.  It also uses less memory.
-
-   Implementation-wise, there are two things to note.
-
- 1. We assume that hash codes are evenly distributed across the
-range of unsigned longs.  The actual hash code stored in the
-entry is left-shifted by 1 bit (losing 1 bit of hash precision),
-and then or'd with 1.  In this way we ensure that the hash field
-of an occupied entry is nonzero.  To map to an index, we
-right-shift the hash by one, divide by the size, and take the
-remainder.
-
- 2. Since the weak references are stored in an atomic region 

Re: Weak tables harmful to GC?

[Ricardo Wurmus]

Hi again,

previously I wrote:

> The “guile-awesome” package finished compiling (after about 46 minutes).
> I’m now testing “guix pull” with a version of Guix that uses
> “guile-awesome”.

I’m sure I’m doing something wrong (see below for guesses).  Here’s what
I get:

--8<---cut here---start->8---
./pre-inst-env guix pull
loading...   26.0% of 645 filesrandom seed for tests: 1509382171
compiling... 18.9% of 645 filesIn thread:
ERROR: In procedure return: return used outside of 'with-monad'Error while 
printing exception.
compiling... 54.7% of 645 files^C
--8<---cut here---end--->8---

I modified build-self.scm to use the modified Guile:

--8<---cut here---start->8---
diff --git a/build-aux/build-self.scm b/build-aux/build-self.scm
index ed8ff5f..9af6504 100644
--- a/build-aux/build-self.scm
+++ b/build-aux/build-self.scm
@@ -126,7 +126,7 @@ running Guile."
   (package->derivation (cond-expand
  (guile-2.2
   (canonical-package
-   (specification->package "guile@2.2")))
+   (specification->package "guile-awesome@2.2")))
  (else
   (canonical-package
(specification->package "guile@2.0"))
--8<---cut here---end--->8---

I also confirmed that the Guile process that is spawned as “bin/guile
--no-auto-compile /home/rwurmus/guix/scripts/guix pull” is indeed the
modified Guile, but I noticed that it spawns yet another Guile process
to load and compile Guix.

I guess that comes from the daemon?  If that’s the case I can’t really
test this on this big server, because the daemon is currently in use, so
I can’t just reconfigure it to use the modified Guile.

When compiling Guix from source with “make -j 32” using that version of
Guile I got a segfault.

--
Ricardo

Re: Weak tables harmful to GC?

[Ricardo Wurmus]

Hi Ludo,

> I’m attaching updated patches.  I’ve let the Guix build run to
> completion this time.  Let me know if it works for you!

The “guile-awesome” package finished compiling (after about 46 minutes).
I’m now testing “guix pull” with a version of Guix that uses
“guile-awesome”.

I’m very hopeful :)

--
Ricardo

Re: Weak tables harmful to GC?

[Ludovic Courtès]

Hi Ricardo,

Ricardo Wurmus  skribis:

> In language/tree-il/analyze.scm:
>   1053:33  3 Exception thrown while printing backtrace:
> ERROR: In procedure assq: Wrong type argument in position 2 (expecting 
> association list): ((system base pmatch) car . #f)
>
> ice-9/boot-9.scm:760:25: In procedure dispatch-exception:
> ice-9/boot-9.scm:760:25: In procedure assq: Wrong type argument in position 2 
> (expecting association list): ((system base pmatch) car . #f)

It’s a sign that the weak tables were too weak, this time.  :-)

The problem stems from the fact that weak pairs were initialized too
late.  Thus, the first calls to ‘scm_weak_car_pair’ were happening
before the weak-car pair GC descriptor had been initialized; they were
therefore using 0 as their descriptor, and ended up not being traced at
all by the GC.

The fix is to initialize weak pairs before symbols, as in 2.0:

modified   libguile/hashtab.c
@@ -1608,10 +1608,11 @@ scm_c_weak_table_fold (scm_t_hash_fold_fn fn, void *closure,
 
 
 
+/* Initialize weak pairs, used by weak hash tables.  This needs to be
+   done early on because it's used by interned symbols etc.  */
 void
-scm_init_hashtab ()
+scm_init_weak_pairs ()
 {
-  /* Initialize weak pairs.  */
   GC_word wcar_pair_bitmap[GC_BITMAP_SIZE (scm_t_cell)] = { 0 };
   GC_word wcdr_pair_bitmap[GC_BITMAP_SIZE (scm_t_cell)] = { 0 };
 
@@ -1627,6 +1628,11 @@ scm_init_hashtab ()
   wcdr_pair_descr = GC_make_descriptor (wcdr_pair_bitmap,
 	GC_WORD_LEN (scm_t_cell));
 
+}
+
+void
+scm_init_hashtab ()
+{
 #include "libguile/hashtab.x"
 }
 
modified   libguile/hashtab.h
@@ -174,6 +174,7 @@ SCM_API SCM scm_hash_map_to_list (SCM proc, SCM hash);
 SCM_API SCM scm_hash_count (SCM hash, SCM pred);
 SCM_INTERNAL void scm_i_hashtable_print (SCM exp, SCM port, scm_print_state *pstate);
 SCM_INTERNAL void scm_init_hashtab (void);
+SCM_INTERNAL void scm_init_weak_pairs (void);
 
 
 /* Guile 2.2.x (x <= 2) weak-table API.  */
modified   libguile/init.c
@@ -390,7 +390,8 @@ scm_i_init_guile (void *base)
 #ifdef GUILE_DEBUG_MALLOC
   scm_debug_malloc_prehistory ();
 #endif
-  scm_symbols_prehistory ();  /* requires weak_table_prehistory */
+  scm_init_weak_pairs ();
+  scm_symbols_prehistory ();  /* requires weak_pairs */
   scm_modules_prehistory ();
   scm_init_array_handle ();

I’m attaching updated patches.  I’ve let the Guix build run to
completion this time.  Let me know if it works for you!

Ludo’.

>From df8dd0b91adc0fca8fafc098eb8db9650f634652 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= 
Date: Sat, 21 Oct 2017 16:18:39 -0600
Subject: [PATCH 1/2] Remove weak tables and revert to weak hash tables.

This removes weak-tables.[ch] and reintroduces weak hash tables as
implemented in Guile 2.0 into hashtab.[ch].  This reduces wall-clock
time by more than 15% on some GC-intensive benchmarks (compiling code)
where big weak hash tables are in use, such as source properties.

For more details on the rationale, see
.

* libguile/weak-table.c, libguile/weak-table.h: Remove.
* libguile.h: Don't include "weak-table.h".
* libguile/Makefile.am (libguile_@GUILE_EFFECTIVE_VERSION@_la_SOURCES)
(DOT_X_FILES, DOT_DOC_FILES, modinclude_HEADERS): Remove weak-table.*
files.
* libguile/evalext.c (scm_self_evaluating_p): Remove reference to
scm_tc7_weak_table.
* libguile/hashtab.c (SCM_HASHTABLEF_WEAK_CAR)
(SCM_HASHTABLEF_WEAK_CDR): New macros.
* libguile/hashtab.c (scm_fixup_weak_alist, vacuum_weak_hash_table)
(do_weak_bucket_fixup, weak_bucket_assoc)
(weak_bucket_assoc_by_hash): New function.
(make_hash_table, scm_make_hash_table): Add support for weak hash
tables.
(weak_gc_callback, weak_gc_hook, weak_gc_finalizer)
(scm_c_register_weak_gc_callback, scm_make_weak_key_hash_table)
(scm_make_weak_value_hash_table, scm_make_doubly_weak_hash_table): New
functions.
(SCM_WEAK_TABLE_P): Remove.
(scm_weak_key_hash_table_p, scm_weak_value_hash_table_p)
(scm_doubly_weak_hash_table_p, scm_hash_fn_get_handle_by_hash): New
functions.
(scm_hash_fn_create_handle_x): Add support for weak hash tables.
(get_weak_cdr, weak_pair_cdr): New functions.
(scm_hash_fn_set_x): Add support for weak hash tables.
(scm_hash_fn_remove_x): Likewise.
(scm_hashq_get_handle, scm_hashq_create_handle_x): Likewise.
(scm_hashv_get_handle, scm_hashv_create_handle_x): Likewise.
(scm_hashq_ref, scm_hashq_set_x, scm_hashq_remove_x): Remove special
cases for 'SCM_WEAK_TABLE_P'.
(scm_hashv_ref, scm_hashv_set_x, scm_hashv_remove_x): Likewise.
(scm_hash_ref, scm_hash_set_x, scm_hash_remove_x): Likewise.
(scm_hashx_ref, scm_hashx_set_x, scm_hashx_remove_x): Likewise.
(assv_predicate, assoc_predicate, assx_predicate): Remove.
(scm_hash_map_to_list, scm_internal_hash_fold): Likewise, and check for
deleted entries.
(scm_internal_hash_for_each_handle): Likewise.
(scm_t_ihashx_closure): Remove 'key' field.
(wcar_pair_descr, 

Re: Weak tables harmful to GC?

[Ricardo Wurmus]

Hi Ludo,

the bootstrap phase now succeeds but the build crashes:

--8<---cut here---start->8---
…
make[2]: Leaving directory 
'/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/bootstrap'
Making all in module
make[2]: Entering directory 
'/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/module'
…
wrote `language/tree-il/spec.go'
  GUILEC srfi/srfi-37.go
wrote `ice-9/textual-ports.go'
wrote `ice-9/time.go'
wrote `ice-9/q.go'
  GUILEC srfi/srfi-38.go
wrote `ice-9/hash-table.go'
wrote `rnrs/r5rs.go'
Backtrace:
In ice-9/eval.scm:
163:9 19 (_ _)
In ice-9/boot-9.scm:
152:2 18 (with-fluid* _ _ _)
In system/base/target.scm:
 57:6 17 (with-target _ _)
In system/base/compile.scm:
152:6 16 (compile-file _ #:output-file _ #:from _ #:to _ #:env _ ?)
 43:4 15 (call-once _)
In ice-9/boot-9.scm:
849:4 14 (with-throw-handler _ _ _)
In system/base/compile.scm:
59:11 13 (_)
   155:11 12 (_ #)
219:8 11 (read-and-compile _ #:from _ #:to _ #:env _ #:opts _)
255:6 10 (compile _ #:from _ #:to _ #:env _ #:opts _)
   183:32  9 (lp (# #) ?)
In language/tree-il/compile-cps.scm:
  1084:25  8 (compile-cps # ?)
974:4  7 (optimize-tree-il # ?)
In language/tree-il/analyze.scm:
563:4  6 (analyze-tree (#< down: # ?) ?)
In srfi/srfi-1.scm:
   656:11  5 (for-each2 (#< down: # ?) ?)
In ice-9/vlist.scm:
   267:16  4 (loop _ _ _)
In language/tree-il/analyze.scm:
  1053:33  3 Exception thrown while printing backtrace:
ERROR: In procedure assq: Wrong type argument in position 2 (expecting 
association list): ((system base pmatch) car . #f)

ice-9/boot-9.scm:760:25: In procedure dispatch-exception:
ice-9/boot-9.scm:760:25: In procedure assq: Wrong type argument in position 2 
(expecting association list): ((system base pmatch) car . #f)
make[2]: *** [Makefile:2258: rnrs/records/procedural.go] Error 1
make[2]: *** Waiting for unfinished jobs
  GUILEC srfi/srfi-41.go
wrote `rnrs/programs.go'
wrote `ice-9/history.go'
wrote `language/elisp/spec.go'
wrote `language/tree-il/optimize.go'
Backtrace:
In ice-9/eval.scm:
163:9 19 (_ _)
In ice-9/boot-9.scm:
152:2 18 (with-fluid* _ _ _)
In system/base/target.scm:
 57:6 17 (with-target _ _)
In system/base/compile.scm:
152:6 16 (compile-file _ #:output-file _ #:from _ #:to _ #:env _ ?)
 43:4 15 (call-once _)
In ice-9/boot-9.scm:
849:4 14 (with-throw-handler _ _ _)
In system/base/compile.scm:
59:11 13 (_)
   155:11 12 (_ #)
219:8 11 (read-and-compile _ #:from _ #:to _ #:env _ #:opts _)
255:6 10 (compile _ #:from _ #:to _ #:env _ #:opts _)
   183:32  9 (lp (# #) ?)
In language/tree-il/compile-cps.scm:
  1084:25  8 (compile-cps _ # _)
974:4  7 (optimize-tree-il # ?)
In language/tree-il/analyze.scm:
563:4  6 (analyze-tree (#< down: # ?) ?)
In srfi/srfi-1.scm:
   656:11  5 (for-each2 (#< down: # ?) ?)
In ice-9/vlist.scm:
   267:16  4 (loop _ _ #t)
In language/tree-il/analyze.scm:
  1053:33  3 Exception thrown while printing backtrace:
ERROR: In procedure assq: Wrong type argument in position 2 (expecting 
association list): 36
wrote `rnrs/unicode.go'

ice-9/boot-9.scm:760:25: In procedure dispatch-exception:
ice-9/boot-9.scm:760:25: In procedure assq: Wrong type argument in position 2 
(expecting association list): 36
wrote `ice-9/curried-definitions.go'
make[2]: *** [Makefile:2258: ice-9/eval.go] Error 1
…
--8<---cut here---end--->8---

This is on the machine with 1.5TB RAM with the same package definition
but using your new patches.

[I’m sending this from my work address, because zoho.com currently has
problems delivering mail to gnu.org.]

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net

Re: Weak tables harmful to GC?

[Ricardo Wurmus]

Hi Ludo,

> The attached patch fixes the bug for me.  Having spend days on
> weak-table stuff, the bug looked (described in the commit log below)
> almost obvious to me.

That’s excellent!  Thank you so much for your efforts!

> Of course, even better if people could test the two patches and confirm
> that it works for them.

I’d like to try this with “guix pull” on the 1.5 TB RAM server.  I’ll
start compiling Guile with these patches tonight and will then configure
Guix to use it.

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net

Re: Weak tables harmful to GC?

[Ludovic Courtès]

Hi,

Ricardo Wurmus  skribis:

>   BOOTSTRAP GUILEC language/tree-il/primitives.go
> /gnu/store/kpxi8h3669afr9r1bgvaf9ij3y4wdyyn-bash-minimal-4.4.12/bin/bash: 
> line 6: 30173 Killed  GUILE_AUTO_COMPILE=0 ../meta/build-env 
> guild compile --target="x86_64-unknown-linux-gnu" -O1 -L 
> "/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/module" -L 
> "/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/guile-readline" -o 
> "ice-9/vlist

Funny: with the “cleanup” that led to the patches you tried, those weak
hash tables were not weak at all, because SCM_WEAK_TABLE_KIND_KEY was
now zero, and thus SCM_HASHTABLE_WEAK_P would always return false.  The
fix:

diff --git a/libguile/hashtab.h b/libguile/hashtab.h
index 8f422b0b5..1705cf744 100644
--- a/libguile/hashtab.h
+++ b/libguile/hashtab.h
@@ -33,6 +33,7 @@
 
 /* Types of weak hash tables.  */
 typedef enum {
+  SCM_WEAK_TABLE_KIND_NONE = 0,
   SCM_WEAK_TABLE_KIND_KEY,
   SCM_WEAK_TABLE_KIND_VALUE,
   SCM_WEAK_TABLE_KIND_BOTH
@@ -51,7 +52,9 @@ typedef enum {
 #define SCM_HASHTABLE_DOUBLY_WEAK_P(x) \
   (SCM_HASHTABLE_FLAGS (x) == SCM_WEAK_TABLE_KIND_BOTH)
 
-#define SCM_HASHTABLE_WEAK_P(x)	   SCM_HASHTABLE_FLAGS (x)
+#define SCM_HASHTABLE_WEAK_P(x)\
+  (SCM_HASHTABLE_FLAGS (x) != SCM_WEAK_TABLE_KIND_NONE)
+
 #define SCM_HASHTABLE_N_ITEMS(x)   (SCM_HASHTABLE (x)->n_items)
 #define SCM_SET_HASHTABLE_N_ITEMS(x, n)   (SCM_HASHTABLE (x)->n_items = n)
 #define SCM_HASHTABLE_INCREMENT(x) (SCM_HASHTABLE_N_ITEMS(x)++)

(Updated patches below.)

With your package definition, I see the first few Guile processes peak
at ~100 MiB resident (would be interesting to compare with stock 2.2.2).

Let me know if it’s better this time!

Thanks again for testing,
Ludo’.

>From eba61a14bd4d39fdfb84e70186b71004044583e3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= 
Date: Sat, 21 Oct 2017 16:18:39 -0600
Subject: [PATCH 1/2] Remove weak tables and revert to weak hash tables.

This removes weak-tables.[ch] and reintroduces weak hash tables as
implemented in Guile 2.0 into hashtab.[ch].  This reduces wall-clock
time by more than 15% on some GC-intensive benchmarks (compiling code)
where big weak hash tables are in use, such as source properties.

For more details on the rationale, see
.

* libguile/weak-table.c, libguile/weak-table.h: Remove.
* libguile.h: Don't include "weak-table.h".
* libguile/Makefile.am (libguile_@GUILE_EFFECTIVE_VERSION@_la_SOURCES)
(DOT_X_FILES, DOT_DOC_FILES, modinclude_HEADERS): Remove weak-table.*
files.
* libguile/evalext.c (scm_self_evaluating_p): Remove reference to
scm_tc7_weak_table.
* libguile/hashtab.c (SCM_HASHTABLEF_WEAK_CAR)
(SCM_HASHTABLEF_WEAK_CDR): New macros.
* libguile/hashtab.c (scm_fixup_weak_alist, vacuum_weak_hash_table)
(do_weak_bucket_fixup, weak_bucket_assoc)
(weak_bucket_assoc_by_hash): New function.
(make_hash_table, scm_make_hash_table): Add support for weak hash
tables.
(weak_gc_callback, weak_gc_hook, weak_gc_finalizer)
(scm_c_register_weak_gc_callback, scm_make_weak_key_hash_table)
(scm_make_weak_value_hash_table, scm_make_doubly_weak_hash_table): New
functions.
(SCM_WEAK_TABLE_P): Remove.
(scm_weak_key_hash_table_p, scm_weak_value_hash_table_p)
(scm_doubly_weak_hash_table_p, scm_hash_fn_get_handle_by_hash): New
functions.
(scm_hash_fn_create_handle_x): Add support for weak hash tables.
(get_weak_cdr, weak_pair_cdr): New functions.
(scm_hash_fn_set_x): Add support for weak hash tables.
(scm_hash_fn_remove_x): Likewise.
(scm_hashq_get_handle, scm_hashq_create_handle_x): Likewise.
(scm_hashv_get_handle, scm_hashv_create_handle_x): Likewise.
(scm_hashq_ref, scm_hashq_set_x, scm_hashq_remove_x): Remove special
cases for 'SCM_WEAK_TABLE_P'.
(scm_hashv_ref, scm_hashv_set_x, scm_hashv_remove_x): Likewise.
(scm_hash_ref, scm_hash_set_x, scm_hash_remove_x): Likewise.
(scm_hashx_ref, scm_hashx_set_x, scm_hashx_remove_x): Likewise.
(assv_predicate, assoc_predicate, assx_predicate): Remove.
(scm_hash_map_to_list, scm_internal_hash_fold): Likewise, and check for
deleted entries.
(scm_internal_hash_for_each_handle): Likewise.
(scm_t_ihashx_closure): Remove 'key' field.
(wcar_pair_descr, wcdr_pair_descr): New variables.
(scm_weak_car_pair, scm_weak_cdr_pair, scm_doubly_weak_pair): New
functions.
(scm_weak_table_refq, scm_weak_table_putq_x, scm_c_make_weak_table)
(scm_c_weak_table_fold): Rewrite in terms of the hash-table API.
(scm_init_hashtab): Initialize 'wcar_pair_descr' and 'wcdr_pair_descr'.
* libguile/hashtab.h (scm_t_weak_table_kind): New type.
(SCM_HASHTABLE, SCM_HASHTABLE_FLAGS, SCM_HASHTABLE_WEAK_KEY_P)
(SCM_HASHTABLE_WEAK_VALUE_P, SCM_HASHTABLE_DOUBLY_WEAK_P): New macros.
(scm_t_hash_predicate_fn): New type.
(scm_t_hashtable)[flags]: New field.
(scm_make_weak_value_hash_table, scm_make_doubly_weak_hash_table)
(scm_make_weak_key_hash_table, scm_c_make_weak_table)
(scm_c_weak_table_fold, 

Re: Weak tables harmful to GC?

[Ludovic Courtès]

Hi Ricardo,

Ricardo Wurmus  skribis:

> I tried building Guile with the following Guix package definition:
>
> (define-public guile-2.2-awesome
>   (package (inherit guile-2.2)
> (name "guile-awesome")
> (source (origin (inherit (package-source guile-2.2))
>(patches (list 
> "/home/rwurmus/0001-Remove-weak-tables-and-revert-to-weak-hash-tables.patch"
>   
> "/home/rwurmus/0002-Keep-weak-hash-table-item-count-consistent.patch"

[...]

>   BOOTSTRAP GUILEC system/foreign.go
> GC Warning: Repeated allocation of very large block (appr. size 230096896):
> May lead to memory leak and poor performance
> GC Warning: Repeated allocation of very large block (appr. size 230096896):
> May lead to memory leak and poor performance
> GC Warning: Repeated allocation of very large block (appr. size 230096896):
> May lead to memory leak and poor performance
> GC Warning: Repeated allocation of very large block (appr. size 230096896):
> May lead to memory leak and poor performance
> GC Warning: Repeated allocation of very large block (appr. size 230096896):
> May lead to memory leak and poor performance
> GC Warning: Repeated allocation of very large block (appr. size 230096896):
> May lead to memory leak and poor performance
> GC Warning: Repeated allocation of very large block (appr. size 230096896):
> May lead to memory leak and poor performance
> Too many heap sections: Increase MAXHINCR or MAX_HEAP_SECTS
> /gnu/store/kpxi8h3669afr9r1bgvaf9ij3y4wdyyn-bash-minimal-4.4.12/bin/bash: 
> line 6: 30796 Aborted GUILE_AUTO_COMPILE=0 ../meta/build-env 
> guild compile --target="x86_64-unknown-linux-gnu" -O1 -L 
> "/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/module" -L 
> "/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/guile-readline" -o 
> "language/scheme/compile-tree-il.go" 
> "../module/language/scheme/compile-tree-il.scm"
> make[2]: *** [Makefile:1928: language/scheme/compile-tree-il.go] Error 134

Blech, that doesn’t sound like an improvement.

“make clean -C module && make” works for me, but now that I think of it
it might be reusing stuff from the prebuilt/ directory.

I’ll try again later.

Thanks for testing,
Ludo’.

Re: Weak tables harmful to GC?

[Ricardo Wurmus]

Hi again,

I tried building this on my workstation with 32GB RAM and the bootstrap
compilation got killed after consuming too much memory.

--8<---cut here---start->8---
…
Making all in bootstrap
make[2]: Entering directory 
'/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/bootstrap'
  BOOTSTRAP GUILEC ice-9/eval.go
wrote `ice-9/eval.go'
  BOOTSTRAP GUILEC ice-9/psyntax-pp.go
  BOOTSTRAP GUILEC language/cps/intmap.go
  BOOTSTRAP GUILEC language/cps/intset.go
  BOOTSTRAP GUILEC language/cps/utils.go
  BOOTSTRAP GUILEC ice-9/vlist.go
  BOOTSTRAP GUILEC srfi/srfi-1.go
  BOOTSTRAP GUILEC language/tree-il.go
  BOOTSTRAP GUILEC language/tree-il/analyze.go
  BOOTSTRAP GUILEC language/tree-il/compile-cps.go
  BOOTSTRAP GUILEC language/tree-il/canonicalize.go
  BOOTSTRAP GUILEC language/tree-il/debug.go
  BOOTSTRAP GUILEC language/tree-il/effects.go
  BOOTSTRAP GUILEC language/tree-il/fix-letrec.go
  BOOTSTRAP GUILEC language/tree-il/optimize.go
  BOOTSTRAP GUILEC language/tree-il/peval.go
  BOOTSTRAP GUILEC language/tree-il/primitives.go
/gnu/store/kpxi8h3669afr9r1bgvaf9ij3y4wdyyn-bash-minimal-4.4.12/bin/bash: line 
6: 30173 Killed  GUILE_AUTO_COMPILE=0 ../meta/build-env guild 
compile --target="x86_64-unknown-linux-gnu" -O1 -L 
"/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/module" -L 
"/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/guile-readline" -o 
"ice-9/vlist.go" "../module/ice-9/vlist.scm"
make[2]: *** [Makefile:1928: ice-9/vlist.go] Error 137
make[2]: *** Waiting for unfinished jobs
…
--8<---cut here---end--->8---

This is still with the same Guix package definition as before:

--8<---cut here---start->8---
(define-public guile-2.2-awesome
  (package (inherit guile-2.2)
(name "guile-awesome")
(source (origin (inherit (package-source guile-2.2))
   (patches (list 
"/home/rwurmus/0001-Remove-weak-tables-and-revert-to-weak-hash-tables.patch"
  
"/home/rwurmus/0002-Keep-weak-hash-table-item-count-consistent.patch"
(arguments
  (substitute-keyword-arguments (package-arguments guile-2.2)
((#:phases phases)
 `(modify-phases ,phases
(add-before 'pre-configure 'bootstrap
  (lambda _
(zero? (system* "autoreconf" "-vif"
(native-inputs
 `(("autoconf" ,autoconf)
   ("automake" ,automake)
   ("libtool" ,libtool)
   ("flex" ,flex)
   ("texinfo" ,texinfo)
   ("gettext" ,gettext-minimal)
   ,@(package-native-inputs guile-2.2)
--8<---cut here---end--->8---


--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net

Re: Weak tables harmful to GC?

[Ricardo Wurmus]

Hi Ludo,

I tried building Guile with the following Guix package definition:

--8<---cut here---start->8---
(define-public guile-2.2-awesome
  (package (inherit guile-2.2)
(name "guile-awesome")
(source (origin (inherit (package-source guile-2.2))
   (patches (list 
"/home/rwurmus/0001-Remove-weak-tables-and-revert-to-weak-hash-tables.patch"
  
"/home/rwurmus/0002-Keep-weak-hash-table-item-count-consistent.patch"
(arguments
  (substitute-keyword-arguments (package-arguments guile-2.2)
((#:phases phases)
 `(modify-phases ,phases
(add-before 'pre-configure 'bootstrap
  (lambda _
(zero? (system* "autoreconf" "-vif"
(native-inputs
 `(("autoconf" ,autoconf)
   ("automake" ,automake)
   ("libtool" ,libtool)
   ("flex" ,flex)
   ("texinfo" ,texinfo)
   ("gettext" ,gettext-minimal)
   ,@(package-native-inputs guile-2.2)
--8<---cut here---end--->8---

Unfortunately, I cannot bootstrap Guile on this 1.5 TB RAM server:

--8<---cut here---start->8---
…
  BOOTSTRAP GUILEC system/vm/program.go
  BOOTSTRAP GUILEC system/vm/vm.go
  BOOTSTRAP GUILEC system/foreign.go
GC Warning: Repeated allocation of very large block (appr. size 230096896):
May lead to memory leak and poor performance
GC Warning: Repeated allocation of very large block (appr. size 230096896):
May lead to memory leak and poor performance
GC Warning: Repeated allocation of very large block (appr. size 230096896):
May lead to memory leak and poor performance
GC Warning: Repeated allocation of very large block (appr. size 230096896):
May lead to memory leak and poor performance
GC Warning: Repeated allocation of very large block (appr. size 230096896):
May lead to memory leak and poor performance
GC Warning: Repeated allocation of very large block (appr. size 230096896):
May lead to memory leak and poor performance
GC Warning: Repeated allocation of very large block (appr. size 230096896):
May lead to memory leak and poor performance
Too many heap sections: Increase MAXHINCR or MAX_HEAP_SECTS
/gnu/store/kpxi8h3669afr9r1bgvaf9ij3y4wdyyn-bash-minimal-4.4.12/bin/bash: line 
6: 30796 Aborted GUILE_AUTO_COMPILE=0 ../meta/build-env guild 
compile --target="x86_64-unknown-linux-gnu" -O1 -L 
"/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/module" -L 
"/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/guile-readline" -o 
"language/scheme/compile-tree-il.go" 
"../module/language/scheme/compile-tree-il.scm"
make[2]: *** [Makefile:1928: language/scheme/compile-tree-il.go] Error 134
make[2]: *** Waiting for unfinished jobs
^GGC Warning: Repeated allocation of very large block (appr. size 230096896):
May lead to memory leak and poor performance
Too many heap sections: Increase MAXHINCR or MAX_HEAP_SECTS
/gnu/store/kpxi8h3669afr9r1bgvaf9ij3y4wdyyn-bash-minimal-4.4.12/bin/bash: line 
6: 30386 Aborted GUILE_AUTO_COMPILE=0 ../meta/build-env guild 
compile --target="x86_64-unknown-linux-gnu" -O1 -L 
"/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/module" -L 
"/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/guile-readline" -o 
"language/tree-il/fix-letrec.go" "../module/language/tree-il/fix-letrec.scm"
make[2]: *** [Makefile:1928: language/tree-il/fix-letrec.go] Error 134
GC Warning: Repeated allocation of very large block (appr. size 230096896):
May lead to memory leak and poor performance
GC Warning: Repeated allocation of very large block (appr. size 230096896):
May lead to memory leak and poor performance
Too many heap sections: Increase MAXHINCR or MAX_HEAP_SECTS
Too many heap sections: Increase MAXHINCR or MAX_HEAP_SECTS
/gnu/store/kpxi8h3669afr9r1bgvaf9ij3y4wdyyn-bash-minimal-4.4.12/bin/bash: line 
6: 30839 Aborted GUILE_AUTO_COMPILE=0 ../meta/build-env guild 
compile --target="x86_64-unknown-linux-gnu" -O1 -L 
"/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/module" -L 
"/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/guile-readline" -o 
"language/value/spec.go" "../module/language/value/spec.scm"
make[2]: *** [Makefile:1928: language/value/spec.go] Error 134
/gnu/store/kpxi8h3669afr9r1bgvaf9ij3y4wdyyn-bash-minimal-4.4.12/bin/bash: line 
6: 30917 Aborted GUILE_AUTO_COMPILE=0 ../meta/build-env guild 
compile --target="x86_64-unknown-linux-gnu" -O1 -L 
"/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/module" -L 
"/tmp/guix-build-guile-awesome-2.2.2.drv-0/guile-2.2.2/guile-readline" -o 
"system/base/syntax.go" "../module/system/base/syntax.scm"
make[2]: *** [Makefile:1928: system/base/syntax.go] Error 134
GC Warning: Repeated allocation of very large block (appr. size 230096896):
May lead to memory leak and poor performance
Too 

Re: Weak tables harmful to GC?

[Ludovic Courtès]

Hello!

Here’s an updated patch set (tested on top of
1008ea315483d1fb41b2a8c10680e511238836d0).

Let me know if things still go wrong.

Thanks,
Ludo’.

>From a301af4f03377c6eabf663df8eeabf6db5e3950a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= 
Date: Sat, 21 Oct 2017 16:18:39 -0600
Subject: [PATCH 1/2] Remove weak tables and revert to weak hash tables.

This removes weak-tables.[ch] and reintroduces weak hash tables as
implemented in Guile 2.0 into hashtab.[ch].  This reduces wall-clock
time by more than 15% on some GC-intensive benchmarks (compiling code)
where big weak hash tables are in use, such as source properties.

For more details on the rationale, see
.

* libguile/weak-table.c, libguile/weak-table.h: Remove.
* libguile.h: Don't include "weak-table.h".
* libguile/Makefile.am (libguile_@GUILE_EFFECTIVE_VERSION@_la_SOURCES)
(DOT_X_FILES, DOT_DOC_FILES, modinclude_HEADERS): Remove weak-table.*
files.
* libguile/evalext.c (scm_self_evaluating_p): Remove reference to
scm_tc7_weak_table.
* libguile/hashtab.c (SCM_HASHTABLEF_WEAK_CAR)
(SCM_HASHTABLEF_WEAK_CDR): New macros.
* libguile/hashtab.c (scm_fixup_weak_alist, vacuum_weak_hash_table)
(do_weak_bucket_fixup, weak_bucket_assoc)
(weak_bucket_assoc_by_hash): New function.
(make_hash_table, scm_make_hash_table): Add support for weak hash
tables.
(weak_gc_callback, weak_gc_hook, weak_gc_finalizer)
(scm_c_register_weak_gc_callback, scm_make_weak_key_hash_table)
(scm_make_weak_value_hash_table, scm_make_doubly_weak_hash_table): New
functions.
(SCM_WEAK_TABLE_P): Remove.
(scm_weak_key_hash_table_p, scm_weak_value_hash_table_p)
(scm_doubly_weak_hash_table_p, scm_hash_fn_get_handle_by_hash): New
functions.
(scm_hash_fn_create_handle_x): Add support for weak hash tables.
(get_weak_cdr, weak_pair_cdr): New functions.
(scm_hash_fn_set_x): Add support for weak hash tables.
(scm_hash_fn_remove_x): Likewise.
(scm_hashq_get_handle, scm_hashq_create_handle_x): Likewise.
(scm_hashv_get_handle, scm_hashv_create_handle_x): Likewise.
(scm_hashq_ref, scm_hashq_set_x, scm_hashq_remove_x): Remove special
cases for 'SCM_WEAK_TABLE_P'.
(scm_hashv_ref, scm_hashv_set_x, scm_hashv_remove_x): Likewise.
(scm_hash_ref, scm_hash_set_x, scm_hash_remove_x): Likewise.
(scm_hashx_ref, scm_hashx_set_x, scm_hashx_remove_x): Likewise.
(assv_predicate, assoc_predicate, assx_predicate): Remove.
(scm_hash_map_to_list, scm_internal_hash_fold): Likewise, and check for
deleted entries.
(scm_internal_hash_for_each_handle): Likewise.
(scm_t_ihashx_closure): Remove 'key' field.
(wcar_pair_descr, wcdr_pair_descr): New variables.
(scm_weak_car_pair, scm_weak_cdr_pair, scm_doubly_weak_pair): New
functions.
(scm_weak_table_refq, scm_weak_table_putq_x, scm_c_make_weak_table)
(scm_c_weak_table_fold): Rewrite in terms of the hash-table API.
(scm_init_hashtab): Initialize 'wcar_pair_descr' and 'wcdr_pair_descr'.
* libguile/hashtab.h (scm_t_weak_table_kind): New type.
(SCM_HASHTABLE, SCM_HASHTABLE_FLAGS, SCM_HASHTABLE_WEAK_KEY_P)
(SCM_HASHTABLE_WEAK_VALUE_P, SCM_HASHTABLE_DOUBLY_WEAK_P): New macros.
(scm_t_hash_predicate_fn): New type.
(scm_t_hashtable)[flags]: New field.
(scm_make_weak_value_hash_table, scm_make_doubly_weak_hash_table)
(scm_make_weak_key_hash_table, scm_c_make_weak_table)
(scm_c_weak_table_fold, scm_weak_table_refq)
(scm_weak_table_putq_x): New declarations.
* libguile/init.c (scm_i_init_guile): Remove calls to
'scm_weak_table_prehistory' and 'scm_init_weak_table'.
(iprin1): Remove reference to scm_tc7_weak_table.
* libguile/procprop.c: Include "hashtab.h".
* libguile/tags.h (scm_tc7_weak_table): Remove.
* libguile/weak-list.h (scm_weak_car_pair, scm_weak_cdr_pair)
(scm_doubly_weak_pair): New declarations.
(SCM_WEAK_PAIR_DELETED_P, SCM_WEAK_PAIR_WORD_DELETED_P)
(SCM_WEAK_PAIR_CAR_DELETED_P, SCM_WEAK_PAIR_CDR_DELETED_P)
(SCM_WEAK_PAIR_WORD, SCM_WEAK_PAIR_CAR, SCM_WEAK_PAIR_CDR): New macros.
* module/system/base/types.scm (%tc7-weak-table): Mark as obsolete.
* test-suite/tests/types.test ("opaque objects"): Replace references to
'weak-table' with 'hash-table'.  Add 'make-hash-table' test.
---
 libguile.h   |3 +-
 libguile/Makefile.am |6 +-
 libguile/evalext.c   |3 +-
 libguile/hashtab.c   |  878 +--
 libguile/hashtab.h   |   47 +-
 libguile/init.c  |4 +-
 libguile/print.c |3 -
 libguile/procprop.c  |4 +-
 libguile/tags.h  |3 +-
 libguile/weak-list.h |   32 +-
 libguile/weak-table.c| 1180 --
 libguile/weak-table.h|   94 
 module/system/base/types.scm |2 +-
 test-suite/tests/types.test  |9 +-
 14 files changed, 807 insertions(+), 1461 deletions(-)
 delete mode 100644 libguile/weak-table.c
 delete mode 100644 libguile/weak-table.h

diff --git a/libguile.h b/libguile.h

Re: Weak tables harmful to GC?

[Ludovic Courtès]

Christopher Allan Webber  skribis:

> Ludovic Courtès writes:
>
>> Christopher Allan Webber  skribis:
>>
>>> Ludovic Courtès writes:
>>>
 Also, it no longer displays the pathological behavior shown in
 .

 Of course, even better if people could test the two patches and confirm
 that it works for them.

 Then if there are no objections I’d like to merge them in ‘stable-2.2’.
>>>
>>> Sounds great indeed, but it didn't apply to master or stable-2.2 for me?
>>
>> Really?  The two patches should apply to stable-2.2, though you need to
>> apply them in the right order (I have it applied over
>> 80696023620eae12f9b2f167aee834f632a32739.)
>>
>> Ludo’.
>
> Huh?  What object is this?  I don't see it in my git repo.

My bad, it’s actually ac0d3dcc533850d25f3e533c04c1c238a83f190b.

Ludo’.

Re: Weak tables harmful to GC?

[Ricardo Wurmus]

Hi Ludo,

does this apply to the latest release of Guile 2.2.2?  I’ve created this
package definition:

--8<---cut here---start->8---
(define-public guile-2.2-awesome
  (package (inherit guile-2.2)
(name "guile-awesome")
(source (origin (inherit (package-source guile-2.2))
   (patches (list 
"0001-Remove-weak-tables-and-revert-to-weak-hash-tables.patch"
  
"0002-Keep-weak-hash-table-item-count-consistent.patch"))

--8<---cut here---end--->8---

But the build fails:

--8<---cut here---start->8---
In file included from weak-table.c:37:0:
../libguile/weak-table.h:34:3: error: redeclaration of enumerator 
'SCM_WEAK_TABLE_KIND_KEY'
   SCM_WEAK_TABLE_KIND_KEY,
   ^
In file included from ../libguile.h:65:0,
 from ../libguile/programs.h:22,
 from ../libguile/_scm.h:85,
 from weak-table.c:30:
../libguile/hashtab.h:36:3: note: previous definition of 
'SCM_WEAK_TABLE_KIND_KEY' was here
   SCM_WEAK_TABLE_KIND_KEY = 1,
   ^
In file included from weak-table.c:37:0:
../libguile/weak-table.h:35:3: error: redeclaration of enumerator 
'SCM_WEAK_TABLE_KIND_VALUE'
   SCM_WEAK_TABLE_KIND_VALUE,
   ^
In file included from ../libguile.h:65:0,
 from ../libguile/programs.h:22,
 from ../libguile/_scm.h:85,
 from weak-table.c:30:
../libguile/hashtab.h:37:3: note: previous definition of 
'SCM_WEAK_TABLE_KIND_VALUE' was here
   SCM_WEAK_TABLE_KIND_VALUE = 2,
   ^
In file included from weak-table.c:37:0:
../libguile/weak-table.h:36:3: error: redeclaration of enumerator 
'SCM_WEAK_TABLE_KIND_BOTH'
   SCM_WEAK_TABLE_KIND_BOTH,
   ^
In file included from ../libguile.h:65:0,
 from ../libguile/programs.h:22,
 from ../libguile/_scm.h:85,
 from weak-table.c:30:
../libguile/hashtab.h:38:3: note: previous definition of 
'SCM_WEAK_TABLE_KIND_BOTH' was here
   SCM_WEAK_TABLE_KIND_BOTH = 1 | 2
   ^
In file included from weak-table.c:37:0:
../libguile/weak-table.h:37:3: error: conflicting types for 
'scm_t_weak_table_kind'
 } scm_t_weak_table_kind;
   ^
In file included from ../libguile.h:65:0,
 from ../libguile/programs.h:22,
 from ../libguile/_scm.h:85,
 from weak-table.c:30:
../libguile/hashtab.h:39:3: note: previous declaration of 
'scm_t_weak_table_kind' was here
 } scm_t_weak_table_kind;
   ^
In file included from weak-table.c:37:0:
../libguile/weak-table.h:46:18: error: conflicting types for 
'scm_c_make_weak_table'
 SCM_INTERNAL SCM scm_c_make_weak_table (unsigned long k,
  ^
In file included from ../libguile.h:65:0,
 from ../libguile/programs.h:22,
 from ../libguile/_scm.h:85,
 from weak-table.c:30:
../libguile/hashtab.h:179:18: note: previous declaration of 
'scm_c_make_weak_table' was here
 SCM_INTERNAL SCM scm_c_make_weak_table (unsigned long k,
  ^
weak-table.c: In function 'make_weak_table':
weak-table.c:798:20: error: 'scm_tc7_weak_table' undeclared (first use in this 
function)
   return scm_cell (scm_tc7_weak_table, (scm_t_bits)table);
^
weak-table.c:798:20: note: each undeclared identifier is reported only once for 
each function it appears in
weak-table.c: At top level:
weak-table.c:848:1: error: conflicting types for 'scm_c_make_weak_table'
 scm_c_make_weak_table (unsigned long k, scm_t_weak_table_kind kind)
 ^
In file included from ../libguile.h:65:0,
 from ../libguile/programs.h:22,
 from ../libguile/_scm.h:85,
 from weak-table.c:30:
../libguile/hashtab.h:179:18: note: previous declaration of 
'scm_c_make_weak_table' was here
 SCM_INTERNAL SCM scm_c_make_weak_table (unsigned long k,
  ^
In file included from ../libguile/_scm.h:81:0,
 from weak-table.c:30:
weak-table.c: In function 'scm_weak_table_p':
weak-table.c:216:47: error: 'scm_tc7_weak_table' undeclared (first use in this 
function)
 #define SCM_WEAK_TABLE_P(x) (SCM_HAS_TYP7 (x, scm_tc7_weak_table))
   ^
../libguile/boolean.h:88:28: note: in definition of macro 'scm_from_bool'
 #define scm_from_bool(x) ((x) ? SCM_BOOL_T : SCM_BOOL_F)
^
../libguile/tags.h:396:34: note: in expansion of macro 'SCM_HAS_HEAP_TYPE'
 #define SCM_HAS_TYP7(x, tag)(SCM_HAS_HEAP_TYPE (x, SCM_TYP7, tag))
  ^
weak-table.c:216:30: note: in expansion of macro 'SCM_HAS_TYP7'
 #define SCM_WEAK_TABLE_P(x) (SCM_HAS_TYP7 (x, scm_tc7_weak_table))
  ^
weak-table.c:864:25: note: in expansion of macro 'SCM_WEAK_TABLE_P'
   return scm_from_bool (SCM_WEAK_TABLE_P (obj));
 ^
In file included from ../libguile/_scm.h:40:0,
 from 

Re: Weak tables harmful to GC?

[Ricardo Wurmus]

[resending this because it may not have arrived]

Ricardo Wurmus  writes:

> Hi Ludo,
>
> does this apply to the latest release of Guile 2.2.2?  I’ve created this
> package definition:
>
> --8<---cut here---start->8---
> (define-public guile-2.2-awesome
>   (package (inherit guile-2.2)
> (name "guile-awesome")
> (source (origin (inherit (package-source guile-2.2))
>(patches (list 
> "0001-Remove-weak-tables-and-revert-to-weak-hash-tables.patch"
>   
> "0002-Keep-weak-hash-table-item-count-consistent.patch"))
>
> --8<---cut here---end--->8---
>
> But the build fails:
>
> --8<---cut here---start->8---
> In file included from weak-table.c:37:0:
> ../libguile/weak-table.h:34:3: error: redeclaration of enumerator 
> 'SCM_WEAK_TABLE_KIND_KEY'
>SCM_WEAK_TABLE_KIND_KEY,
>^
> In file included from ../libguile.h:65:0,
>  from ../libguile/programs.h:22,
>  from ../libguile/_scm.h:85,
>  from weak-table.c:30:
> ../libguile/hashtab.h:36:3: note: previous definition of 
> 'SCM_WEAK_TABLE_KIND_KEY' was here
>SCM_WEAK_TABLE_KIND_KEY = 1,
>^
> In file included from weak-table.c:37:0:
> ../libguile/weak-table.h:35:3: error: redeclaration of enumerator 
> 'SCM_WEAK_TABLE_KIND_VALUE'
>SCM_WEAK_TABLE_KIND_VALUE,
>^
> In file included from ../libguile.h:65:0,
>  from ../libguile/programs.h:22,
>  from ../libguile/_scm.h:85,
>  from weak-table.c:30:
> ../libguile/hashtab.h:37:3: note: previous definition of 
> 'SCM_WEAK_TABLE_KIND_VALUE' was here
>SCM_WEAK_TABLE_KIND_VALUE = 2,
>^
> In file included from weak-table.c:37:0:
> ../libguile/weak-table.h:36:3: error: redeclaration of enumerator 
> 'SCM_WEAK_TABLE_KIND_BOTH'
>SCM_WEAK_TABLE_KIND_BOTH,
>^
> In file included from ../libguile.h:65:0,
>  from ../libguile/programs.h:22,
>  from ../libguile/_scm.h:85,
>  from weak-table.c:30:
> ../libguile/hashtab.h:38:3: note: previous definition of 
> 'SCM_WEAK_TABLE_KIND_BOTH' was here
>SCM_WEAK_TABLE_KIND_BOTH = 1 | 2
>^
> In file included from weak-table.c:37:0:
> ../libguile/weak-table.h:37:3: error: conflicting types for 
> 'scm_t_weak_table_kind'
>  } scm_t_weak_table_kind;
>^
> In file included from ../libguile.h:65:0,
>  from ../libguile/programs.h:22,
>  from ../libguile/_scm.h:85,
>  from weak-table.c:30:
> ../libguile/hashtab.h:39:3: note: previous declaration of 
> 'scm_t_weak_table_kind' was here
>  } scm_t_weak_table_kind;
>^
> In file included from weak-table.c:37:0:
> ../libguile/weak-table.h:46:18: error: conflicting types for 
> 'scm_c_make_weak_table'
>  SCM_INTERNAL SCM scm_c_make_weak_table (unsigned long k,
>   ^
> In file included from ../libguile.h:65:0,
>  from ../libguile/programs.h:22,
>  from ../libguile/_scm.h:85,
>  from weak-table.c:30:
> ../libguile/hashtab.h:179:18: note: previous declaration of 
> 'scm_c_make_weak_table' was here
>  SCM_INTERNAL SCM scm_c_make_weak_table (unsigned long k,
>   ^
> weak-table.c: In function 'make_weak_table':
> weak-table.c:798:20: error: 'scm_tc7_weak_table' undeclared (first use in 
> this function)
>return scm_cell (scm_tc7_weak_table, (scm_t_bits)table);
> ^
> weak-table.c:798:20: note: each undeclared identifier is reported only once 
> for each function it appears in
> weak-table.c: At top level:
> weak-table.c:848:1: error: conflicting types for 'scm_c_make_weak_table'
>  scm_c_make_weak_table (unsigned long k, scm_t_weak_table_kind kind)
>  ^
> In file included from ../libguile.h:65:0,
>  from ../libguile/programs.h:22,
>  from ../libguile/_scm.h:85,
>  from weak-table.c:30:
> ../libguile/hashtab.h:179:18: note: previous declaration of 
> 'scm_c_make_weak_table' was here
>  SCM_INTERNAL SCM scm_c_make_weak_table (unsigned long k,
>   ^
> In file included from ../libguile/_scm.h:81:0,
>  from weak-table.c:30:
> weak-table.c: In function 'scm_weak_table_p':
> weak-table.c:216:47: error: 'scm_tc7_weak_table' undeclared (first use in 
> this function)
>  #define SCM_WEAK_TABLE_P(x) (SCM_HAS_TYP7 (x, scm_tc7_weak_table))
>^
> ../libguile/boolean.h:88:28: note: in definition of macro 'scm_from_bool'
>  #define scm_from_bool(x) ((x) ? SCM_BOOL_T : SCM_BOOL_F)
> ^
> ../libguile/tags.h:396:34: note: in expansion of macro 'SCM_HAS_HEAP_TYPE'
>  #define SCM_HAS_TYP7(x, tag)(SCM_HAS_HEAP_TYPE (x, SCM_TYP7, tag))
>   ^
> weak-table.c:216:30: note: in expansion of macro 

Re: Weak tables harmful to GC?

[Christopher Allan Webber]

Ludovic Courtès writes:

> Christopher Allan Webber  skribis:
>
>> Ludovic Courtès writes:
>>
>>> Also, it no longer displays the pathological behavior shown in
>>> .
>>>
>>> Of course, even better if people could test the two patches and confirm
>>> that it works for them.
>>>
>>> Then if there are no objections I’d like to merge them in ‘stable-2.2’.
>>
>> Sounds great indeed, but it didn't apply to master or stable-2.2 for me?
>
> Really?  The two patches should apply to stable-2.2, though you need to
> apply them in the right order (I have it applied over
> 80696023620eae12f9b2f167aee834f632a32739.)
>
> Ludo’.

Huh?  What object is this?  I don't see it in my git repo.

This is the latest commit I see to stable-2.2, which is also what
Savannah sees:

  
https://git.savannah.gnu.org/cgit/guile.git/commit/?h=stable-2.2=a74d4ee4f6e062ff640f2532c9cfc9977bb68a49

Re: Weak tables harmful to GC?

[Ludovic Courtès]

Christopher Allan Webber  skribis:

> Ludovic Courtès writes:
>
>> Also, it no longer displays the pathological behavior shown in
>> .
>>
>> Of course, even better if people could test the two patches and confirm
>> that it works for them.
>>
>> Then if there are no objections I’d like to merge them in ‘stable-2.2’.
>
> Sounds great indeed, but it didn't apply to master or stable-2.2 for me?

Really?  The two patches should apply to stable-2.2, though you need to
apply them in the right order (I have it applied over
80696023620eae12f9b2f167aee834f632a32739.)

Ludo’.

Re: Weak tables harmful to GC?

[Christopher Allan Webber]

Ludovic Courtès writes:

> Also, it no longer displays the pathological behavior shown in
> .
>
> Of course, even better if people could test the two patches and confirm
> that it works for them.
>
> Then if there are no objections I’d like to merge them in ‘stable-2.2’.

Sounds great indeed, but it didn't apply to master or stable-2.2 for me?

Re: Weak tables harmful to GC?

[Ludovic Courtès]

Hi Chris,

Thanks for your support.  :-)

Christopher Allan Webber  skribis:

> +  else
> +{
> +  /* The move to BDW-GC with Guile 2.0 introduced some bugs
> + related to weak hash tables, threads, memory usage, and the
> + alloc lock.  We were unable to fix these issues
> + satisfactorily in 2.0 but have addressed them via a rewrite
> + in 2.2.  If you see this message often, you probably want
> + to upgrade to 2.2.  */
> +  fprintf (stderr, "guile: warning: weak hash table corruption "
> +   "(https://bugs.gnu.org/19180)");
> +  len = 0;
> +}
>
> Guess reverting this patch means this comment also should be amended!

Indeed.  :-)

The attached patch fixes the bug for me.  Having spend days on
weak-table stuff, the bug looked (described in the commit log below)
almost obvious to me.

This tight loop used to trigger the bug after a few seconds; it no
longer does after several minutes:

  (define table
(make-weak-key-hash-table))

  (let loop ((i 0))
(unless #f
  (hashq-set! table (make-list 1000) i)
  (loop (1+ i

Also, it no longer displays the pathological behavior shown in
.

Of course, even better if people could test the two patches and confirm
that it works for them.

Then if there are no objections I’d like to merge them in ‘stable-2.2’.

Ludo’.

>From d61b9c0768ef28965be61e6c160f56bd17ef8715 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= 
Date: Sun, 22 Oct 2017 16:56:51 -0700
Subject: [PATCH 2/2] Keep weak hash table item count consistent.

Fixes a TOCTTOU kind of bug whereby we'd first count the number of items
deleted from the table, and later, *without* having the alloc lock, we'd
update the table's item count.  The problem is that the item count could
have been updated in the meantime, hence the bug.

Fixes .

* libguile/hashtab.c (vacuum_weak_hash_table): Rename to...
(do_vacuum_weak_hash_table): ... this.  Unmarshall the void* argument.
Replace 'fprintf' warning with an assertion.
(vacuum_weak_hash_table): New function.  Call the above with
'GC_call_with_alloc_lock'.
(t_fixup_args): Add 'table' field; remove 'removed_items'.
(do_weak_bucket_fixup): Update TABLE's 'n_items' field.
(weak_bucket_assoc): Check 'SCM_HASHTABLE_N_ITEMS' instead of
'args.removed_items'.
---
 libguile/hashtab.c | 68 +++---
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/libguile/hashtab.c b/libguile/hashtab.c
index bd308c5e9..7a6585740 100644
--- a/libguile/hashtab.c
+++ b/libguile/hashtab.c
@@ -96,7 +96,7 @@ static char *s_hashtable = "hashtable";
 
 /* Remove nullified weak pairs from ALIST such that the result contains only
valid pairs.  Set REMOVED_ITEMS to the number of pairs that have been
-   deleted.  */
+   deleted.  Assumes the allocation lock is already taken.  */
 static SCM
 scm_fixup_weak_alist (SCM alist, size_t *removed_items)
 {
@@ -130,9 +130,10 @@ scm_fixup_weak_alist (SCM alist, size_t *removed_items)
   return result;
 }
 
-static void
-vacuum_weak_hash_table (SCM table)
+static void *
+do_vacuum_weak_hash_table (void *arg)
 {
+  SCM table = SCM_PACK_POINTER (arg);
   SCM buckets = SCM_HASHTABLE_VECTOR (table);
   unsigned long k = SCM_SIMPLE_VECTOR_LENGTH (buckets);
   size_t len = SCM_HASHTABLE_N_ITEMS (table);
@@ -142,44 +143,52 @@ vacuum_weak_hash_table (SCM table)
   size_t removed;
   SCM alist = SCM_SIMPLE_VECTOR_REF (buckets, k);
   alist = scm_fixup_weak_alist (alist, );
-  if (removed <= len)
-len -= removed;
-  else
-{
-  /* The move to BDW-GC with Guile 2.0 introduced some bugs
- related to weak hash tables, threads, memory usage, and the
- alloc lock.  We were unable to fix these issues
- satisfactorily in 2.0 but have addressed them via a rewrite
- in 2.2.  If you see this message often, you probably want
- to upgrade to 2.2.  */
-  fprintf (stderr, "guile: warning: weak hash table corruption "
-   "(https://bugs.gnu.org/19180)\n");
-  len = 0;
-}
+
+  /* The alloc lock is taken, so we cannot get REMOVED > LEN.  If we
+	 do, that means we messed up while counting items.  */
+  assert (removed <= len);
+
   SCM_SIMPLE_VECTOR_SET (buckets, k, alist);
 }
 
   SCM_SET_HASHTABLE_N_ITEMS (table, len);
+
+  return table;
+}
+
+/* Remove deleted weak pairs from the buckets of TABLE, and update
+   TABLE's item count accordingly.  */
+static void
+vacuum_weak_hash_table (SCM table)
+{
+  /* Take the alloc lock so we have a consistent view of the live
+ elements in TABLE.  Failing to do that, we could be miscounting the
+ number of elements.  */
+  GC_call_with_alloc_lock (do_vacuum_weak_hash_table,
+			   

Re: Weak tables harmful to GC?

[Christopher Allan Webber]

+  else
+{
+  /* The move to BDW-GC with Guile 2.0 introduced some bugs
+ related to weak hash tables, threads, memory usage, and the
+ alloc lock.  We were unable to fix these issues
+ satisfactorily in 2.0 but have addressed them via a rewrite
+ in 2.2.  If you see this message often, you probably want
+ to upgrade to 2.2.  */
+  fprintf (stderr, "guile: warning: weak hash table corruption "
+   "(https://bugs.gnu.org/19180)");
+  len = 0;
+}

Guess reverting this patch means this comment also should be amended!

Re: Weak tables harmful to GC?

[Ludovic Courtès]

Hello!

l...@gnu.org (Ludovic Courtès) skribis:

> All in all, given that these issues are very likely causes of the
> execution time and memory consumption issues that plague the compiler
> (where we have huge symbol and source property tables), I’m in favor of
> switching back to the 2.0 implementation of weak hash tables.  That can
> be done in an API-compatible way, I think.

The patch below reverts to the 2.0 bucket-and-weak-lists weak hash
tables.

On my benchmark, which calls ‘emit-bytecode’ on a huge piece of code
(corresponding to (gnu packages python)), it runs ~18% faster than
current 2.2; memory usage is reduced by only ~3% at the end.

It appears to work well except under stress: a tight loop as in
 easily triggers the “weak hash table
corruption” message or an assertion failure.

To be continued…

Ludo’.

>From 49cb10e6f054a202ffc47109d0f9d88df9546b70 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= 
Date: Sat, 21 Oct 2017 16:18:39 -0600
Subject: [PATCH] Remove weak tables and revert to weak hash tables.

This removes weak-tables.[ch] and reintroduces weak hash tables as
implemented in Guile 2.0 into hashtab.[ch].  This reduces wall-clock
time by more than 15% on some GC-intensive benchmarks (compiling code)
where big weak hash tables are in use, such as source properties.

For more details on the rationale, see
.

* libguile.h: Don't include "weak-table.h".
* libguile/Makefile.am (libguile_@GUILE_EFFECTIVE_VERSION@_la_SOURCES)
(DOT_X_FILES, DOT_DOC_FILES, modinclude_HEADERS): Remove weak-table.*
files.
* libguile/evalext.c (scm_self_evaluating_p): Remove reference to
scm_tc7_weak_table.
* libguile/hashtab.c (SCM_HASHTABLEF_WEAK_CAR)
(SCM_HASHTABLEF_WEAK_CDR): New macros.
* libguile/hashtab.c (scm_fixup_weak_alist, vacuum_weak_hash_table)
(do_weak_bucket_fixup, weak_bucket_assoc)
(weak_bucket_assoc_by_hash): New function.
(make_hash_table, scm_make_hash_table): Add support for weak hash
tables.
(weak_gc_callback, weak_gc_hook, weak_gc_finalizer)
(scm_c_register_weak_gc_callback, scm_make_weak_key_hash_table)
(scm_make_weak_value_hash_table, scm_make_doubly_weak_hash_table): New
functions.
(SCM_WEAK_TABLE_P): Remove.
(scm_weak_key_hash_table_p, scm_weak_value_hash_table_p)
(scm_doubly_weak_hash_table_p, scm_hash_fn_get_handle_by_hash): New
functions.
(scm_hash_fn_create_handle_x): Add support for weak hash tables.
(get_weak_cdr, weak_pair_cdr): New functions.
(scm_hash_fn_set_x): Add support for weak hash tables.
(scm_hash_fn_remove_x): Likewise.
(scm_hashq_get_handle, scm_hashq_create_handle_x): Likewise.
(scm_hashv_get_handle, scm_hashv_create_handle_x): Likewise.
(scm_hashq_ref, scm_hashq_set_x, scm_hashq_remove_x): Remove special
cases for 'SCM_WEAK_TABLE_P'.
(scm_hashv_ref, scm_hashv_set_x, scm_hashv_remove_x): Likewise.
(scm_hash_ref, scm_hash_set_x, scm_hash_remove_x): Likewise.
(scm_hashx_ref, scm_hashx_set_x, scm_hashx_remove_x): Likewise.
(assv_predicate, assoc_predicate, assx_predicate): Remove.
(scm_hash_map_to_list, scm_internal_hash_fold): Likewise, and check for
deleted entries.
(scm_internal_hash_for_each_handle): Likewise.
(scm_t_ihashx_closure): Remove 'key' field.
(wcar_pair_descr, wcdr_pair_descr): New variables.
(scm_weak_car_pair, scm_weak_cdr_pair, scm_doubly_weak_pair): New
functions.
(scm_weak_table_refq, scm_weak_table_putq_x, scm_c_make_weak_table)
(scm_c_weak_table_fold): Rewrite in terms of the hash-table API.
(scm_init_hashtab): Initialize 'wcar_pair_descr' and 'wcdr_pair_descr'.
* libguile/hashtab.h (scm_t_weak_table_kind): New type.
(SCM_HASHTABLE, SCM_HASHTABLE_FLAGS, SCM_HASHTABLE_WEAK_KEY_P)
(SCM_HASHTABLE_WEAK_VALUE_P, SCM_HASHTABLE_DOUBLY_WEAK_P): New macros.
(scm_t_hash_predicate_fn): New type.
(scm_t_hashtable)[flags]: New field.
(scm_make_weak_value_hash_table, scm_make_doubly_weak_hash_table)
(scm_make_weak_key_hash_table, scm_c_make_weak_table)
(scm_c_weak_table_fold, scm_weak_table_refq)
(scm_weak_table_putq_x): New declarations.
* libguile/init.c (scm_i_init_guile): Remove calls to
'scm_weak_table_prehistory' and 'scm_init_weak_table'.
(iprin1): Remove reference to scm_tc7_weak_table.
* libguile/procprop.c: Include "hashtab.h".
* libguile/tags.h (scm_tc7_weak_table): Remove.
* libguile/weak-list.h (scm_weak_car_pair, scm_weak_cdr_pair)
(scm_doubly_weak_pair): New declarations.
(SCM_WEAK_PAIR_DELETED_P, SCM_WEAK_PAIR_WORD_DELETED_P)
(SCM_WEAK_PAIR_CAR_DELETED_P, SCM_WEAK_PAIR_CDR_DELETED_P)
(SCM_WEAK_PAIR_WORD, SCM_WEAK_PAIR_CAR, SCM_WEAK_PAIR_CDR): New macros.
* module/system/base/types.scm (%tc7-weak-table): Mark as obsolete.
* test-suite/tests/types.test ("opaque objects"): Replace references to
'weak-table' with 'hash-table'.  Add 'make-hash-table' test.
---
 libguile.h   |   3 +-
 libguile/Makefile.am |   6 +-
 libguile/evalext.c   |   3 +-
 libguile/hashtab.c   | 878 

Re: Weak tables harmful to GC?

[Christopher Allan Webber]

Ludovic Courtès writes:

> I’ve come to the conclusion that the 2.2 weak-table implementation
> strategy cannot work efficiently with libgc.
>
> I’m also skeptical (perhaps that’s also because I’m insufficiently
> informed, tell me!) about the “open-addressed” strategy that is used.
> To me, it’s necessarily less space-efficient than a regular hash table
> with chaining since we always have at least 10% more weak entries than
> the number of actual entries in the table (and in practice it’s usually
> much more than 10% AFAICS, because of the gap between subsequent sizes.)
>
> All in all, given that these issues are very likely causes of the
> execution time and memory consumption issues that plague the compiler
> (where we have huge symbol and source property tables), I’m in favor of
> switching back to the 2.0 implementation of weak hash tables.  That can
> be done in an API-compatible way, I think.

If ~reverting this one thing will fix our current major problems
(especially if api compatibility can be preserved), I would say it's a
good idea for now.  Presumably including a re-iteration of the weak hash
tables that include the new work could be still done in the future once
these problems are solved?

Thanks for all your hard work on this Ludo'!

Weak tables harmful to GC?

[Ludovic Courtès]

Hello,

l...@gnu.org (Ludovic Courtès) skribis:

> Total time: 66.515425859 seconds (31.859444991 seconds in GC)

So, more investigation on the GC side…

‘perf’ shows a profile like this:

--8<---cut here---start->8---
  12.33%  guilelibgc.so.1.0.3 [.] GC_mark_from
  10.67%  guilelibgc.so.1.0.3 [.] 
GC_move_disappearing_link_inner
   8.47%  guilelibgc.so.1.0.3 [.] GC_header_cache_miss
   7.06%  guilelibgc.so.1.0.3 [.] GC_mark_and_push
   5.98%  guilelibgc.so.1.0.3 [.] GC_finalize
   4.08%  guilelibpthread-2.25.so [.] pthread_mutex_trylock
   4.03%  guilelibgc.so.1.0.3 [.] 
GC_register_disappearing_link_inner
   3.95%  guilelibpthread-2.25.so [.] 
__pthread_mutex_unlock_usercnt
   3.07%  guilelibguile-2.2.so.1.2.0  [.] weak_table_put_x
   3.05%  guilelibgc.so.1.0.3 [.] GC_base
   2.49%  guilelibguile-2.2.so.1.2.0  [.] resize_table
   2.47%  guilelibgc.so.1.0.3 [.] GC_is_marked
   1.76%  guilelibguile-2.2.so.1.2.0  [.] rob_from_rich
   1.54%  guilelibgc.so.1.0.3 [.] GC_grow_table
   1.22%  guilelibgc.so.1.0.3 [.] GC_find_header
   1.19%  guilelibgc.so.1.0.3 [.] GC_clear_mark_bit
   1.17%  guilelibguile-2.2.so.1.2.0  [.] mem2uinteger
   1.13%  guilelibgc.so.1.0.3 [.] GC_malloc_kind
   0.98%  guilelibguile-2.2.so.1.2.0  [.] peek_byte_or_eof
   0.98%  guilelibguile-2.2.so.1.2.0  [.] scm_getc
   0.71%  guilelibguile-2.2.so.1.2.0  [.] scm_get_byte_or_eof
   0.68%  guilelibguile-2.2.so.1.2.0  [.] scm_to_uint64
   0.68%  guilelibguile-2.2.so.1.2.0  [.] read_token
   0.67%  guilelibgc.so.1.0.3 [.] GC_is_heap_ptr
   0.64%  guilelibguile-2.2.so.1.2.0  [.] scm_unget_bytes
   0.60%  guilelibguile-2.2.so.1.2.0  [.] read_inner_expression
   0.59%  guilelibguile-2.2.so.1.2.0  [.] scm_flush
   0.58%  guilelibguile-2.2.so.1.2.0  [.] peek_utf8_codepoint
   0.55%  guilelibguile-2.2.so.1.2.0  [.] scm_set_cdr_x
   0.53%  guilelibgc.so.1.0.3 [.] GC_push_marked
   0.53%  guilelibguile-2.2.so.1.2.0  [.] scm_c_weak_set_lookup
   0.51%  guilelibgc.so.1.0.3 [.] GC_call_with_alloc_lock
   0.51%  guilelibguile-2.2.so.1.2.0  [.] scm_read_sexp
   0.50%  guilelibguile-2.2.so.1.2.0  [.] mark_weak_key_table
   0.49%  guilelibguile-2.2.so.1.2.0  [.] move_weak_entry.part.0
   0.49%  guilelibguile-2.2.so.1.2.0  [.] scm_ungetc
   0.47%  guilelibguile-2.2.so.1.2.0  [.] scm_to_int32
   0.46%  guilelibguile-2.2.so.1.2.0  [.] flush_ws
   0.45%  guilelibguile-2.2.so.1.2.0  [.] scm_i_string_ref
   0.41%  guilelibgc.so.1.0.3 [.] GC_reclaim_clear
   0.39%  guilelibgc.so.1.0.3 [.] GC_move_disappearing_link
--8<---cut here---end--->8---

As you hinted on #guix a while back Andy, the mark procedures Guile uses
break libgc’s expectations.  Specifically, ‘mark_weak_key_table’
typically pushes lots of objects on the mark stack (in my example the
source properties table has a 3595271 ‘scm_t_weak_entry’ objects, so at
every mark phase, we push roughly all of that on the mark stack.)

Internally, libgc would never do that: in ‘GC_mark_from’ it has a
limited “credit” for marking, and it stops when it has consumed all of
it.  However, with mark procedures, it cannot do that because the mark
procedure obviously keeps running until it’s done, and from libgc’s
viewpoint, the mark procedure marks one object (in this case, the big
weak entry array.)

(Besides, libgc recommends against using mark procedures in the first
place, but we cannot use the GC_DS_BITMAP approach here because it only
works for objects up to 30 words, not 200+ MiB arrays…)

In addition to this, ‘GC_move_disappearing_link’ is expensive, as shown
in the profile, yet it’s called quite a lot on table resizing.


I’ve come to the conclusion that the 2.2 weak-table implementation
strategy cannot work efficiently with libgc.

I’m also skeptical (perhaps that’s also because I’m insufficiently
informed, tell me!) about the “open-addressed” strategy that is used.
To me, it’s necessarily less space-efficient than a regular hash table
with chaining since we always have at least 10% more weak entries than
the number of actual entries in the table (and in practice it’s usually
much more than 10% AFAICS, because of the gap between subsequent sizes.)


All in all, given that these issues are very likely causes of the
execution time and memory consumption issues that plague the compiler
(where we have huge symbol and