[PATCH] Revise GC asserts.

[PATCH] Revise GC asserts.

[Han-Wen Nienhuys]

* libguile/gc.c (scm_i_gc): Change assert into deprecation warning.

* libguile/private-gc.h (nil): introduce scm_i_last_marked_cell_count,
  as a private mechanism for maintaining cell counts.  Previous
  versions incremented scm_cells_allocated in an inlined function, so
  loading dynamic objects of older GUILEs would break invariants.
---
 libguile/gc.c         |   18 +++++++++++++-----
 libguile/private-gc.h |    5 +++--
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/libguile/gc.c b/libguile/gc.c
index f3ef585..0323f87 100644
--- a/libguile/gc.c
+++ b/libguile/gc.c
@@ -416,7 +416,7 @@ gc_end_stats ()
 
   scm_gc_cells_allocated_acc +=
     (double) scm_i_gc_sweep_stats.collected;
-  scm_gc_cells_marked_acc += (double) scm_cells_allocated;
+  scm_gc_cells_marked_acc += (double) scm_i_last_marked_cell_count;
   scm_gc_cells_marked_conservatively_acc += (double) scm_i_find_heap_calls;
   scm_gc_cells_swept_acc += (double) scm_i_gc_sweep_stats.swept;
 
@@ -558,6 +558,8 @@ scm_check_deprecated_memory_return ()
   scm_i_deprecated_memory_return = 0;
 }
 
+long int scm_i_last_marked_cell_count;
+
 /* Must be called while holding scm_i_sweep_mutex.
 
    This function is fairly long, but it touches various global
@@ -603,11 +605,17 @@ scm_i_gc (const char *what)
   /* TODO(hanwen): figure out why the stats are off on x64_64. */
   /* If this was not true, someone touched mark bits outside of the
      mark phase. */
-  assert (scm_cells_allocated == scm_i_marked_count ());
+  if (scm_i_last_marked_cell_count != scm_i_marked_count ())
+    {
+      static char msg[] =
+	"The number of marked objects changed since the last GC. "
+	"Are you marking objects outside of the mark phase?";
+      scm_c_issue_deprecation_warning(msg);
+    }
   assert (scm_i_gc_sweep_stats.swept
 	  == (scm_i_master_freelist.heap_total_cells
 	      + scm_i_master_freelist2.heap_total_cells));
-  assert (scm_i_gc_sweep_stats.collected + scm_cells_allocated
+  assert (scm_i_gc_sweep_stats.collected + scm_i_last_marked_cell_count
 	  == scm_i_gc_sweep_stats.swept);
 #endif /* SCM_DEBUG_CELL_ACCESSES */
   
@@ -617,8 +625,8 @@ scm_i_gc (const char *what)
   scm_mark_all ();
   scm_gc_mark_time_taken += (scm_c_get_internal_run_time () - t_before_gc);
 
-  scm_cells_allocated = scm_i_marked_count ();
- 
+  scm_i_last_marked_cell_count = scm_cells_allocated = scm_i_marked_count ();
+
   /* Sweep
 
     TODO: the after_sweep hook should probably be moved to just before
diff --git a/libguile/private-gc.h b/libguile/private-gc.h
index 93503ce..f5331ab 100644
--- a/libguile/private-gc.h
+++ b/libguile/private-gc.h
@@ -273,8 +273,9 @@ SCM_INTERNAL void scm_i_sweep_all_segments (char const *reason,
 SCM_INTERNAL SCM scm_i_all_segments_statistics (SCM hashtab);
 SCM_INTERNAL unsigned long *scm_i_segment_table_info(int *size);
 
-extern long int scm_i_deprecated_memory_return;
-extern long int scm_i_find_heap_calls;
+SCM_INTERNAL long int scm_i_deprecated_memory_return;
+SCM_INTERNAL long int scm_i_find_heap_calls;
+SCM_INTERNAL long int scm_i_last_marked_cell_count;
 
 /*
   global init funcs.
-- 
1.5.5.1

Re: [PATCH] Revise GC asserts.

[Ludovic Courtès]

Hi,

Han-Wen Nienhuys <hanwen@xs4all.nl> writes:

> * libguile/gc.c (scm_i_gc): Change assert into deprecation warning.

Why?  It's not a deprecation but really an invariant, right?

> * libguile/private-gc.h (nil): introduce scm_i_last_marked_cell_count,
>   as a private mechanism for maintaining cell counts.  Previous
>   versions incremented scm_cells_allocated in an inlined function, so
>   loading dynamic objects of older GUILEs would break invariants.

OTOH, if we are to change the way `scm_cells_allocated' is used and
don't want older code to interfere with that, it's safe the break the
ABI here (we're on `master' after all).

Thus I would vote in favor of making `scm_cells_allocated' internal
(which requires that no public macro or inline function refer to it) or
renaming it, e.g., to `scm_i_cells_allocated'.

BTW, can you add a one-line summary to the log, as is done on `master',
`vm', etc.?

Thanks!

Ludo'.

Re: [PATCH] Revise GC asserts.

[Han-Wen Nienhuys]

Ludovic Courtès escreveu:
>> * libguile/gc.c (scm_i_gc): Change assert into deprecation warning.
> 
> Why?  It's not a deprecation but really an invariant, right?

Yes, but it probably does not warrant crashing the program; memory 
allocation sizes will just be a bit off as a result.

> Thus I would vote in favor of making `scm_cells_allocated' internal
> (which requires that no public macro or inline function refer to it) or
> renaming it, e.g., to `scm_i_cells_allocated'.

Let's just remove the variable, since scm_i_last_marked_cell_count is a
more exact name.

> BTW, can you add a one-line summary to the log, as is done on `master',
> `vm', etc.?

The custom is (see git-format-patch) that the subject line is the summary.

-- 
 Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

Re: [PATCH] Revise GC asserts.

[Ludovic Courtès]

Hello,

Han-Wen Nienhuys <hanwen@xs4all.nl> writes:

> Ludovic Courtès escreveu:
>>> * libguile/gc.c (scm_i_gc): Change assert into deprecation warning.
>> 
>> Why?  It's not a deprecation but really an invariant, right?
>
> Yes, but it probably does not warrant crashing the program; memory 
> allocation sizes will just be a bit off as a result.

While the thing is under development, it probably makes sense to have an
`assert ()'.  Hitting these assertions wouldn't be a problem if there
was a separate GC development branch.  In the meantime, `printf ()'
seems better suited than `scm_c_issue_deprecation_warning ()'.

> Let's just remove the variable, since scm_i_last_marked_cell_count is a
> more exact name.

OK.

Thanks,
Ludo'.

Re: [PATCH] Revise GC asserts.

[Han-Wen Nienhuys]

Ludovic Courtès escreveu:
>> * libguile/private-gc.h (nil): introduce scm_i_last_marked_cell_count,
>>   as a private mechanism for maintaining cell counts.  Previous
>>   versions incremented scm_cells_allocated in an inlined function, so
>>   loading dynamic objects of older GUILEs would break invariants.
> 
> OTOH, if we are to change the way `scm_cells_allocated' is used and
> don't want older code to interfere with that, it's safe the break the
> ABI here (we're on `master' after all).

If this is our stance, this looks like a good opportunity to take all
these variable private, and have people use gc-stats to get the data.

Providing direct access to variables is problematic from an API stability
point of view.

-- 
 Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

Re: [PATCH] Revise GC asserts.

[Ludovic Courtès]

Hello,

Han-Wen Nienhuys <hanwen@xs4all.nl> writes:

> If this is our stance, this looks like a good opportunity to take all
> these variable private, and have people use gc-stats to get the data.
>
> Providing direct access to variables is problematic from an API stability
> point of view.

I agree.  I'll support you if you take this route.  :-)

Thanks,
Ludo'.