Raymond Toy pushed to branch issue-243-weak-pointer-to-static-array at cmucl / cmucl

Commits: 9346ebc1 by Raymond Toy at 2024-03-01T08:15:23-08:00 Revert "Update some comments; slightly modify scan_static_vectors to use pop"

This reverts commit 1f8a6328ba47804660e1811a8227522d86be0259.

- - - - - cf831e08 by Raymond Toy at 2024-03-01T08:15:41-08:00 Revert "Clear static vector visited bit when scavenging a weak pointer to it"

This reverts commit 0bcba23ea0319d26552af1766273a61a5fd6c0cd.

- - - - - 275348c4 by Raymond Toy at 2024-03-01T08:16:04-08:00 Revert "Remove unmark_static_vectors_in_use and inuse_static_vector_list"

This reverts commit 081dcba913e565de7a0fbcf0ad68113fec78c1e7.

- - - - - b4017a0b by Raymond Toy at 2024-03-01T08:16:22-08:00 Revert "Don't need inuse list anymore"

This reverts commit 8ae61cf30fe5e2f09df91e90f814adf1c75fef20.

- - - - - fed6581f by Raymond Toy at 2024-03-01T08:16:40-08:00 Revert "First cut at clearing the mark during phase 1"

This reverts commit c3be06f3c38118aef6137dae1df956fdf689e93a.

- - - - -

1 changed file:

- src/lisp/gencgc.c

Changes:

===================================== src/lisp/gencgc.c ===================================== @@ -2130,6 +2130,7 @@ static lispobj(*transother[256]) (lispobj object); static int (*sizetab[256]) (lispobj * where);

static struct weak_pointer *weak_pointers; +static struct weak_pointer *inuse_static_vector_list; static struct scavenger_hook *scavenger_hooks = (struct scavenger_hook *) NIL;

/* Like (ceiling x y), but y is constrained to be a power of two */ @@ -2734,7 +2735,7 @@ scav_static_vector(lispobj object) int static_p;

if (debug_static_array_p) { - fprintf(stderr, "Possible static vector at %p. header = 0x%08lx\n", + fprintf(stderr, "Possible static vector at %p. header = 0x%lx\n", ptr, (unsigned long) header); }

@@ -2745,9 +2746,9 @@ scav_static_vector(lispobj object) * setting the MSB of the header. And clear out any * possible visited bit. */ - *ptr = (header | STATIC_VECTOR_MARK_BIT); + *ptr = (header | STATIC_VECTOR_MARK_BIT) & ~STATIC_VECTOR_VISITED_BIT; if (debug_static_array_p) { - fprintf(stderr, "Scavenged static vector @%p, header = 0x%08lx\n", + fprintf(stderr, "Scavenged static vector @%p, header = 0x%lx\n", ptr, (unsigned long) header); } } @@ -5388,14 +5389,6 @@ scav_weak_pointer(lispobj * where, lispobj object) struct weak_pointer *this_wp = (struct weak_pointer *) where;

if (this_wp->mark_bit == NIL) { - lispobj *header = (lispobj *) PTR(this_wp->value); - if (maybe_static_array_p(*header)) { - printf("Scavenge wp %p to static array %p header 0x%08lx\n", - this_wp, (lispobj *) this_wp->value, *header); - - *header &= ~STATIC_VECTOR_VISITED_BIT; - } - this_wp->mark_bit = T; this_wp->next = weak_pointers; weak_pointers = this_wp; @@ -5449,14 +5442,16 @@ push_weak_pointer(struct weak_pointer* wp, struct weak_pointer **list) }

/* - * Phase 2: Process the list of weak pointers to unused static - * vectors. We find the unique static vectors and add each - * corresponding weak pointer to freeable_list. For the duplicates, - * the weak pointer is broken. + * Phase 2: Find the weak pointers to static vectors that are in use + * and not in use. The vectors that are in use are added to + * inuse_list. Those that are not in use are added to freeable_list. + * Only unique vectors are added to freeable list; a duplicate has its + * weak pointer to it broken. */ static void scan_static_vectors_2(struct weak_pointer *static_vector_list, - struct weak_pointer **freeable_list) + struct weak_pointer **freeable_list, + struct weak_pointer **inuse_list) { DPRINTF(debug_static_array_p, (stdout, "Phase 2: Find unused and unused static vectors\n")); @@ -5473,23 +5468,34 @@ scan_static_vectors_2(struct weak_pointer *static_vector_list, (stdout, " wp %p value %p header 0x%08lx\n", wp, (lispobj *) wp->value, *header));

- /* - * If we haven't seen this vector before, set the visited flag - * and add it to freeable_list. If we have visited this - * vector before, break the weak pointer. - */ - if ((*header & STATIC_VECTOR_VISITED_BIT) == 0) { + if ((*header & STATIC_VECTOR_MARK_BIT) != 0) { + /* + * Static vector is in use. Add this to the in-use list. + */ DPRINTF(debug_static_array_p, - (stdout, " Visit unused vector, add to freeable list\n")); + (stdout, " In-use vector; add to in-use list\n"));

- *header |= STATIC_VECTOR_VISITED_BIT; - push_weak_pointer(wp, freeable_list); + push_weak_pointer(wp, inuse_list); } else { - DPRINTF(debug_static_array_p, - (stdout, " Already visited unused vector; break weak pointer\n")); + /* + * Static vector not in use. If we haven't seen this + * vector before, set the visited flag and add it to + * freeable_list. If we have visited this vector before, + * break the weak pointer. + */ + if ((*header & STATIC_VECTOR_VISITED_BIT) == 0) { + DPRINTF(debug_static_array_p, + (stdout, " Visit unused vector, add to freeable list\n"));

- wp->value = NIL; - wp->broken = T; + *header |= STATIC_VECTOR_VISITED_BIT; + push_weak_pointer(wp, freeable_list); + } else { + DPRINTF(debug_static_array_p, + (stdout, " Already visited unused vector; break weak pointer\n")); + + wp->value = NIL; + wp->broken = T; + } } }

@@ -5512,15 +5518,18 @@ scan_static_vectors_3(struct weak_pointer *freeable_list) lispobj *header = (lispobj *) PTR(wp->value); lispobj *static_array = (lispobj *) PTR(wp->value);

+ /* + * Invariant: weak pointer must not be broken + */ + gc_assert(wp->broken == NIL); + DPRINTF(debug_static_array_p, (stdout, " wp %p value %p header 0x%08lx\n", wp, (lispobj*) wp->value, *header));

/* - * Invariants: weak pointer must not be broken and the mark - * bit must be clear. + * Invariant: Mark bit must be clear */ - gc_assert(wp->broken == NIL); gc_assert(((*header & STATIC_VECTOR_MARK_BIT) == 0));

DPRINTF(debug_static_array_p, @@ -5536,6 +5545,49 @@ scan_static_vectors_3(struct weak_pointer *freeable_list) (stdout, "Phase 3 done\n")); }

+/* + * Unmark all the vectors in inuse_list. This needs to be called at + * the end of GC to unmark any live static vectors so that for the + * next GC we can tell if the static vector is used or not. + * Otherwise, the vectors will always look as if they're in use + * because the mark bit is never changed. + */ +static void +unmark_static_vectors_in_use(struct weak_pointer *inuse_list) +{ + struct weak_pointer *wp; + + DPRINTF(debug_static_array_p, + (stdout, "Phase 4: unmark static vectors\n")); + + for (wp = inuse_list; wp; wp = wp->next) { + lispobj *header = (lispobj *) PTR(wp->value); + /* + * Invariant: the weak pointer must not be broken. + * + * Note that we can't assert that the static vector is marked + * because we can have more than one weak pointer to the same + * static vector. + */ + gc_assert(wp->broken == NIL); + + DPRINTF(debug_static_array_p, + (stdout, " wp %p value %p broken %d header 0x%08lx\n", + wp, (lispobj*) wp->value, wp->broken == T, *header)); + + /* Only clear if we haven't already */ + if ((*header & STATIC_VECTOR_MARK_BIT) != 0) { + DPRINTF(debug_static_array_p, + (stdout, " Clearing mark bit\n")); + + *header &= ~STATIC_VECTOR_MARK_BIT; + } + } + + DPRINTF(debug_static_array_p, + (stdout, "Phase 4 done\n")); +} + static void scan_static_vectors(struct weak_pointer *static_vector_list) { @@ -5543,10 +5595,11 @@ scan_static_vectors(struct weak_pointer *static_vector_list) struct weak_pointer *freeable_list = NULL;

/* - * For each weak pointer, either break it, or add it to the - * freeable list because it points to a unique static vector. + * For each weak pointer, add it either the inuse list or the + * freeable list. */ - scan_static_vectors_2(static_vector_list, &freeable_list); + inuse_static_vector_list = NULL; + scan_static_vectors_2(static_vector_list, &freeable_list, &inuse_static_vector_list);

/* Free the unused unique static vectors. */ scan_static_vectors_3(freeable_list); @@ -5564,18 +5617,15 @@ scan_weak_pointers(void) * * Also find any weak pointers to static vectors. This * destructively modifies the next slot of the weak pointer to - * chain all the weak pointers to unused static vectors together. + * chain all the weak pointers to static vectors together. */ DPRINTF(debug_static_array_p, (stdout, "Phase 1: Process weak pointers\n"));

- while (weak_pointers) { - lispobj value; - lispobj *first_pointer; - - wp = pop_weak_pointer(&weak_pointers); - value = wp->value; - first_pointer = (lispobj *) PTR(value); + while (wp) { + struct weak_pointer *next = wp->next; + lispobj value = wp->value; + lispobj *first_pointer = (lispobj *) PTR(value);

wp->mark_bit = NIL; if (Pointerp(value)) { @@ -5588,29 +5638,23 @@ scan_weak_pointers(void) } } else { /* The value may be a static vector */ - lispobj *header = (lispobj *) PTR(value); - - if (maybe_static_array_p(*header)) { - if (*header & STATIC_VECTOR_MARK_BIT) { - DPRINTF(debug_static_array_p, - (stdout, " Update status bits for live vector: wp %p value %p header 0x%08lx\n", - wp, (lispobj *) wp->value, *header)); - *header = (*header & ~STATIC_VECTOR_MARK_BIT) | STATIC_VECTOR_VISITED_BIT; - } else if ((*header & STATIC_VECTOR_VISITED_BIT) == 0) { - /* Only add the vector if it is unmarked and has not been visited */ - DPRINTF(debug_static_array_p, - (stdout, " Add static vector: wp %p value %p header 0x%08lx\n", - wp, (lispobj *) wp->value, *header)); - - push_weak_pointer(wp, &static_vector_list); - } + lispobj header = *(lispobj *) PTR(value); + + if (maybe_static_array_p(header)) { + + DPRINTF(debug_static_array_p, + (stdout, " Add static vector: wp %p value %p header 0x%08lx\n", + wp, (lispobj *) wp->value, header)); + + push_weak_pointer(wp, &static_vector_list); } else { DPRINTF(debug_static_array_p, (stdout, " Skip: wp %p value %p header 0x%08lx\n", - wp, (lispobj *) wp->value, *header)); + wp, (lispobj *) wp->value, header)); } } } + wp = next; }

scan_static_vectors(static_vector_list); @@ -8346,6 +8390,13 @@ collect_garbage(unsigned last_gen) } scavenger_hooks = (struct scavenger_hook *) NIL; } + + /* + * Unmark any live static vectors. This needs to be done at the + * very end when all GCs are done, lest we accidentally free a + * static vector that was actually in use. + */ + unmark_static_vectors_in_use(inuse_static_vector_list); }

View it on GitLab: https://gitlab.common-lisp.net/cmucl/cmucl/-/compare/1f8a6328ba47804660e1811...