freeing srcprops ?

freeing srcprops ?

[Han-Wen Nienhuys]

Hello,

I'm doign some leak checking to see why LilyPond needs such obscene
amounts of memory. Looking through the valgrind leak table, I see mention 
of srcprops. In srcprop.c, I see:  


static size_t
srcprops_free (SCM obj)
{
  *((scm_t_srcprops **) SCM_SMOB_DATA (obj)) = srcprops_freelist;
  srcprops_freelist = (scm_t_srcprops *) SCM_SMOB_DATA (obj);
  return 0; /* srcprops_chunks are not freed until leaving guile */
}


why use a separate storage pool for srcprop objects?  If optimized storage
is needed, isn't it better to compress the srcprop a bit and use a 
double cell?


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



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Han-Wen Nienhuys]

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

Han-Wen Nienhuys escreveu:
> Hello,
> 
> I'm doign some leak checking to see why LilyPond needs such obscene
> amounts of memory. Looking through the valgrind leak table, I see mention 
> of srcprops. In srcprop.c, I see:  
> 
> 
> static size_t
> srcprops_free (SCM obj)
> {
>   *((scm_t_srcprops **) SCM_SMOB_DATA (obj)) = srcprops_freelist;
>   srcprops_freelist = (scm_t_srcprops *) SCM_SMOB_DATA (obj);
>   return 0; /* srcprops_chunks are not freed until leaving guile */
> }
> 
> 
> why use a separate storage pool for srcprop objects?  If optimized storage
> is needed, isn't it better to compress the srcprop a bit and use a 
> double cell?

Please have a look at the attached patch.  Test suite is OK. I can't detect any
performance changes introduced by this patch (I tried timing loading all of 
ice-9/ )


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

[-- Attachment #2: 0001-srcprops-cleanup.txt --]
[-- Type: text/plain, Size: 8998 bytes --]

>From f8dd4c337da81db3e7e1f3faf2663ec67f496580 Mon Sep 17 00:00:00 2001
From: Han-Wen Nienhuys <hanwen@lilypond.org>
Date: Thu, 18 Jan 2007 15:00:44 +0100
Subject: [PATCH] srcprops cleanup.

- remove macros without SCM_ prefix from srcprops.h

- rename PROCTRACEP to SCM_PROCTRACEP

- Use double cell for storing srcprops.

- Get rid of specialized srcprop storage.
---
 libguile/eval.c    |    2 +-
 libguile/srcprop.c |  108 +++++++++++++++++++--------------------------------
 libguile/srcprop.h |   40 +------------------
 3 files changed, 43 insertions(+), 107 deletions(-)

diff --git a/libguile/eval.c b/libguile/eval.c
index 26d90f1..d797fc0 100644
--- a/libguile/eval.c
+++ b/libguile/eval.c
@@ -3024,7 +3024,7 @@ scm_eval_body (SCM code, SCM env)
 do { \
   SCM_SET_ARGSREADY (debug);\
   if (scm_check_apply_p && SCM_TRAPS_P)\
-    if (SCM_APPLY_FRAME_P || (SCM_TRACE_P && PROCTRACEP (proc)))\
+    if (SCM_APPLY_FRAME_P || (SCM_TRACE_P && SCM_PROCTRACEP (proc)))\
       {\
 	SCM tmp, tail = scm_from_bool(SCM_TRACED_FRAME_P (debug)); \
 	SCM_SET_TRACED_FRAME (debug); \
diff --git a/libguile/srcprop.c b/libguile/srcprop.c
index e1b8673..b44b503 100644
--- a/libguile/srcprop.c
+++ b/libguile/srcprop.c
@@ -37,7 +37,7 @@
 /* {Source Properties}
  *
  * Properties of source list expressions.
- * Five of these have special meaning and optimized storage:
+ * Five of these have special meaning:
  *
  * filename    string   The name of the source file.
  * copy        list     A copy of the list expression.
@@ -55,29 +55,47 @@ SCM_GLOBAL_SYMBOL (scm_sym_line, "line");
 SCM_GLOBAL_SYMBOL (scm_sym_column, "column");
 SCM_GLOBAL_SYMBOL (scm_sym_breakpoint, "breakpoint");
 
+
+
+/*
+  layout:
+  
+  car = tag
+  cbr = pos
+  ccr = copy
+  cdr = plist 
+*/
+
+#define SRCPROPSP(p) (SCM_SMOB_PREDICATE (scm_tc16_srcprops, (p)))
+#define SRCPROPBRK(p) (SCM_SMOB_FLAGS (p) & SCM_SOURCE_PROPERTY_FLAG_BREAK)
+#define SRCPROPPOS(p) (SCM_CELL_WORD(p,1))
+#define SRCPROPLINE(p) (SRCPROPPOS(p) >> 12)
+#define SRCPROPCOL(p) (SRCPROPPOS(p) & 0x0fffL)
+#define SRCPROPCOPY(p) (SCM_CELL_OBJECT(p,2))
+#define SRCPROPPLIST(p) (SCM_CELL_OBJECT_3(p))
+#define SETSRCPROPBRK(p) \
+ (SCM_SET_SMOB_FLAGS ((p), \
+                      SCM_SMOB_FLAGS (p) | SCM_SOURCE_PROPERTY_FLAG_BREAK))
+#define CLEARSRCPROPBRK(p)  \
+ (SCM_SET_SMOB_FLAGS ((p), \
+                      SCM_SMOB_FLAGS (p) & ~SCM_SOURCE_PROPERTY_FLAG_BREAK))
+#define SRCPROPMAKPOS(l, c) (((l) << 12) + (c))
+#define SETSRCPROPPOS(p, l, c) (SCM_SET_CELL_WORD(p,1, SRCPROPMAKPOS (l, c)))
+#define SETSRCPROPLINE(p, l) SETSRCPROPPOS (p, l, SRCPROPCOL (p))
+#define SETSRCPROPCOL(p, c) SETSRCPROPPOS (p, SRCPROPLINE (p), c)
+
+
+
 scm_t_bits scm_tc16_srcprops;
-static scm_t_srcprops_chunk *srcprops_chunklist = 0;
-static scm_t_srcprops *srcprops_freelist = 0;
 
 
 static SCM
 srcprops_mark (SCM obj)
 {
-  scm_gc_mark (SRCPROPFNAME (obj));
   scm_gc_mark (SRCPROPCOPY (obj));
   return SRCPROPPLIST (obj);
 }
 
-
-static size_t
-srcprops_free (SCM obj)
-{
-  *((scm_t_srcprops **) SCM_SMOB_DATA (obj)) = srcprops_freelist;
-  srcprops_freelist = (scm_t_srcprops *) SCM_SMOB_DATA (obj);
-  return 0; /* srcprops_chunks are not freed until leaving guile */
-}
-
-
 static int
 srcprops_print (SCM obj, SCM port, scm_print_state *pstate)
 {
@@ -99,38 +117,17 @@ scm_c_source_property_breakpoint_p (SCM form)
 }
 
 
+
 SCM
 scm_make_srcprops (long line, int col, SCM filename, SCM copy, SCM plist)
 {
-  register scm_t_srcprops *ptr;
-  SCM_CRITICAL_SECTION_START;
-  if ((ptr = srcprops_freelist) != NULL)
-    srcprops_freelist = *(scm_t_srcprops **)ptr;
-  else
-    {
-      size_t i;
-      scm_t_srcprops_chunk *mem;
-      size_t n = sizeof (scm_t_srcprops_chunk)
-	            + sizeof (scm_t_srcprops) * (SRCPROPS_CHUNKSIZE - 1);
-      SCM_SYSCALL (mem = (scm_t_srcprops_chunk *) scm_malloc (n));
-      if (mem == NULL)
-	scm_memory_error ("srcprops");
-      scm_gc_register_collectable_memory (mem, n, "srcprops");
-      
-      mem->next = srcprops_chunklist;
-      srcprops_chunklist = mem;
-      ptr = &mem->srcprops[0];
-      for (i = 1; i < SRCPROPS_CHUNKSIZE - 1; ++i)
-	*(scm_t_srcprops **)&ptr[i] = &ptr[i + 1];
-      *(scm_t_srcprops **)&ptr[SRCPROPS_CHUNKSIZE - 1] = 0;
-      srcprops_freelist = (scm_t_srcprops *) &ptr[1];
-    }
-  ptr->pos = SRCPROPMAKPOS (line, col);
-  ptr->fname = filename;
-  ptr->copy = copy;
-  ptr->plist = plist;
-  SCM_CRITICAL_SECTION_END;
-  SCM_RETURN_NEWSMOB (scm_tc16_srcprops, ptr);
+  if (!SCM_UNBNDP (filename))
+    plist = scm_acons (scm_sym_filename, filename, plist);
+  
+  SCM_RETURN_NEWSMOB3 (scm_tc16_srcprops,
+		       SRCPROPMAKPOS (line, col),
+		       copy,
+		       plist);
 }
 
 
@@ -140,8 +137,6 @@ scm_srcprops_to_plist (SCM obj)
   SCM plist = SRCPROPPLIST (obj);
   if (!SCM_UNBNDP (SRCPROPCOPY (obj)))
     plist = scm_acons (scm_sym_copy, SRCPROPCOPY (obj), plist);
-  if (!SCM_UNBNDP (SRCPROPFNAME (obj)))
-    plist = scm_acons (scm_sym_filename, SRCPROPFNAME (obj), plist);
   plist = scm_acons (scm_sym_column, scm_from_int (SRCPROPCOL (obj)), plist);
   plist = scm_acons (scm_sym_line, scm_from_int (SRCPROPLINE (obj)), plist);
   plist = scm_acons (scm_sym_breakpoint, scm_from_bool (SRCPROPBRK (obj)), plist);
@@ -206,7 +201,6 @@ SCM_DEFINE (scm_source_property, "source-property", 2, 0, 0,
   if      (scm_is_eq (scm_sym_breakpoint, key)) p = scm_from_bool (SRCPROPBRK (p));
   else if (scm_is_eq (scm_sym_line,       key)) p = scm_from_int (SRCPROPLINE (p));
   else if (scm_is_eq (scm_sym_column,     key)) p = scm_from_int (SRCPROPCOL (p));
-  else if (scm_is_eq (scm_sym_filename,   key)) p = SRCPROPFNAME (p);
   else if (scm_is_eq (scm_sym_copy,       key)) p = SRCPROPCOPY (p);
   else
     {
@@ -277,13 +271,6 @@ SCM_DEFINE (scm_set_source_property_x, "set-source-property!", 3, 0, 0,
 		      scm_make_srcprops (0, scm_to_int (datum),
 					 SCM_UNDEFINED, SCM_UNDEFINED, p));
     }
-  else if (scm_is_eq (scm_sym_filename, key))
-    {
-      if (SRCPROPSP (p))
-	SRCPROPFNAME (p) = datum;
-      else
-	SCM_WHASHSET (scm_source_whash, h, scm_make_srcprops (0, 0, datum, SCM_UNDEFINED, p));
-    }
   else if (scm_is_eq (scm_sym_copy, key))
     {
       if (SRCPROPSP (p))
@@ -308,7 +295,6 @@ scm_init_srcprop ()
 {
   scm_tc16_srcprops = scm_make_smob_type ("srcprops", 0);
   scm_set_smob_mark (scm_tc16_srcprops, srcprops_mark);
-  scm_set_smob_free (scm_tc16_srcprops, srcprops_free);
   scm_set_smob_print (scm_tc16_srcprops, srcprops_print);
 
   scm_source_whash = scm_make_weak_key_hash_table (scm_from_int (2047));
@@ -317,20 +303,6 @@ scm_init_srcprop ()
 #include "libguile/srcprop.x"
 }
 
-void
-scm_finish_srcprop ()
-{
-  register scm_t_srcprops_chunk *ptr = srcprops_chunklist, *next;
-  size_t n= sizeof (scm_t_srcprops_chunk)
-    + sizeof (scm_t_srcprops) * (SRCPROPS_CHUNKSIZE - 1);
-  while (ptr)
-    {
-      next = ptr->next;
-      scm_gc_unregister_collectable_memory (ptr, n, "srcprops");
-      free ((char *) ptr);
-      ptr = next;
-    }
-}
 
 /*
   Local Variables:
diff --git a/libguile/srcprop.h b/libguile/srcprop.h
index c0e4277..87e5fde 100644
--- a/libguile/srcprop.h
+++ b/libguile/srcprop.h
@@ -49,46 +49,10 @@ do { \
 
 /* {Source properties}
  */
-
-SCM_API scm_t_bits scm_tc16_srcprops;
-
-typedef struct scm_t_srcprops
-{
-  unsigned long pos;
-  SCM fname;
-  SCM copy;
-  SCM plist;
-} scm_t_srcprops;
-
-#define SRCPROPS_CHUNKSIZE 2047 /* Number of srcprops per chunk */
-typedef struct scm_t_srcprops_chunk
-{
-  struct scm_t_srcprops_chunk *next;
-  scm_t_srcprops srcprops[1];
-} scm_t_srcprops_chunk;
-
+#define SCM_PROCTRACEP(x) (scm_is_true (scm_procedure_property (x, scm_sym_trace)))
 #define SCM_SOURCE_PROPERTY_FLAG_BREAK 1
 
-#define SRCPROPSP(p) (SCM_SMOB_PREDICATE (scm_tc16_srcprops, (p)))
-#define SRCPROPBRK(p) (SCM_SMOB_FLAGS (p) & SCM_SOURCE_PROPERTY_FLAG_BREAK)
-#define SRCPROPPOS(p) ((scm_t_srcprops *) SCM_SMOB_DATA (p))->pos
-#define SRCPROPLINE(p) (SRCPROPPOS(p) >> 12)
-#define SRCPROPCOL(p) (SRCPROPPOS(p) & 0x0fffL)
-#define SRCPROPFNAME(p) ((scm_t_srcprops *) SCM_SMOB_DATA (p))->fname
-#define SRCPROPCOPY(p) ((scm_t_srcprops *) SCM_SMOB_DATA (p))->copy
-#define SRCPROPPLIST(p) ((scm_t_srcprops *) SCM_SMOB_DATA (p))->plist
-#define SETSRCPROPBRK(p) \
- (SCM_SET_SMOB_FLAGS ((p), \
-                      SCM_SMOB_FLAGS (p) | SCM_SOURCE_PROPERTY_FLAG_BREAK))
-#define CLEARSRCPROPBRK(p)  \
- (SCM_SET_SMOB_FLAGS ((p), \
-                      SCM_SMOB_FLAGS (p) & ~SCM_SOURCE_PROPERTY_FLAG_BREAK))
-#define SRCPROPMAKPOS(l, c) (((l) << 12) + (c))
-#define SETSRCPROPPOS(p, l, c) (SRCPROPPOS (p) = SRCPROPMAKPOS (l, c))
-#define SETSRCPROPLINE(p, l) SETSRCPROPPOS (p, l, SRCPROPCOL (p))
-#define SETSRCPROPCOL(p, c) SETSRCPROPPOS (p, SRCPROPLINE (p), c)
-
-#define PROCTRACEP(x) (scm_is_true (scm_procedure_property (x, scm_sym_trace)))
+SCM_API scm_t_bits scm_tc16_srcprops;
 
 SCM_API SCM scm_sym_filename;
 SCM_API SCM scm_sym_copy;
-- 
1.4.4.2


[-- Attachment #3: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Kevin Ryde]

Han-Wen Nienhuys <hanwen@xs4all.nl> writes:
>
>  SCM
>  scm_make_srcprops (long line, int col, SCM filename, SCM copy, SCM plist)
>  {
> +  if (!SCM_UNBNDP (filename))
> +    plist = scm_acons (scm_sym_filename, filename, plist);

Can those two cells be shared among all source props for the same
file, to save space?

> +  SCM_RETURN_NEWSMOB3 (scm_tc16_srcprops,
> +		       SRCPROPMAKPOS (line, col),

If col is a freaky big value then perhaps put it in the plist.  Could
be helpful if there's stupidly long lines in some generated code file,
wouldn't cost anything normally.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Han-Wen Nienhuys]

Kevin Ryde escreveu:
> Han-Wen Nienhuys <hanwen@xs4all.nl> writes:
>>  SCM
>>  scm_make_srcprops (long line, int col, SCM filename, SCM copy, SCM plist)
>>  {
>> +  if (!SCM_UNBNDP (filename))
>> +    plist = scm_acons (scm_sym_filename, filename, plist);
> 
> Can those two cells be shared among all source props for the same
> file, to save space?

Not the list cell, but the pair-cell is sharable, at the cost of 
some infrastructure for sharing it.

>> +  SCM_RETURN_NEWSMOB3 (scm_tc16_srcprops,
>> +		       SRCPROPMAKPOS (line, col),
> 
> If col is a freaky big value then perhaps put it in the plist.  Could
> be helpful if there's stupidly long lines in some generated code file,
> wouldn't cost anything normally.

It costs in terms of  infrastructure and code to deal with this. 
SInce this is for debugging, a freaky big value doesn't make sense at 
all, IMO. 



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



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Han-Wen Nienhuys]

Kevin Ryde escreveu:
> Han-Wen Nienhuys <hanwen@xs4all.nl> writes:
>>  SCM
>>  scm_make_srcprops (long line, int col, SCM filename, SCM copy, SCM plist)
>>  {
>> +  if (!SCM_UNBNDP (filename))
>> +    plist = scm_acons (scm_sym_filename, filename, plist);
> 
> Can those two cells be shared among all source props for the same
> file, to save space?

see below.

Hmmm, on 2nd thought, there is a race condition in this code.  Lemme see.

>> +  SCM_RETURN_NEWSMOB3 (scm_tc16_srcprops,
>> +		       SRCPROPMAKPOS (line, col),
> 
> If col is a freaky big value then perhaps put it in the plist.  Could

col is 0x0FFF max, which 4096. Seems plenty for me. 

We could of course increase this limit, but then the max number of
lines goes down from its current value of 1M.


commit 1e8cecc70617d69bd93e8c4761aedf1008f454f6
Author: Han-Wen Nienhuys <hanwen@lilypond.org>
Date:   Fri Jan 19 12:46:57 2007 +0100

    Reuse last filename acons if possible.
    
    This saves on memory use, since plist usually is SCM_EOL.

diff --git a/libguile/srcprop.c b/libguile/srcprop.c
index b44b503..8d61272 100644
--- a/libguile/srcprop.c
+++ b/libguile/srcprop.c
@@ -88,7 +88,6 @@ SCM_GLOBAL_SYMBOL (scm_sym_breakpoint, "breakpoint");
 
 scm_t_bits scm_tc16_srcprops;
 
-
 static SCM
 srcprops_mark (SCM obj)
 {
@@ -117,12 +116,33 @@ scm_c_source_property_breakpoint_p (SCM form)
 }
 
 
+/*
+  A protected cells whose cdr contains the last plist
+  used if plist contains only the filename.
+
+  This works because scm_set_source_property_x does
+  not use assoc-set! for modifying the plist.
+ */
+static SCM scm_last_plist_filename;
 
 SCM
 scm_make_srcprops (long line, int col, SCM filename, SCM copy, SCM plist)
 {
   if (!SCM_UNBNDP (filename))
-    plist = scm_acons (scm_sym_filename, filename, plist);
+    {
+      SCM old_plist = plist;
+      if (old_plist == SCM_EOL
+	  && SCM_CDADR (scm_last_plist_filename) == filename)
+	{
+	  plist = SCM_CDR (scm_last_plist_filename);
+	}
+      else
+	{
+	  plist = scm_acons (scm_sym_filename, filename, plist);
+	  if (old_plist == SCM_EOL)
+	    SCM_SETCDR (scm_last_plist_filename, plist);
+	}
+    }
   
   SCM_RETURN_NEWSMOB3 (scm_tc16_srcprops,
 		       SRCPROPMAKPOS (line, col),
@@ -300,6 +320,10 @@ scm_init_srcprop ()
   scm_source_whash = scm_make_weak_key_hash_table (scm_from_int (2047));
   scm_c_define ("source-whash", scm_source_whash);
 
+  scm_last_plist_filename
+    = scm_permanent_object (scm_cons (SCM_EOL,
+				      scm_acons (SCM_EOL, SCM_EOL, SCM_EOL)));
+
 #include "libguile/srcprop.x"
 }
 


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



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Kevin Ryde]

Han-Wen Nienhuys <hanwen@xs4all.nl> writes:
>
> col is 0x0FFF max, which 4096. Seems plenty for me. 

ice-9/psyntax.pp overflows it.  You need to make sure unusually big
values don't result in garbage, even if the storage is optimized for
sensible or usual cases.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Han-Wen Nienhuys]

Kevin Ryde escreveu:
> Han-Wen Nienhuys <hanwen@xs4all.nl> writes:
>> col is 0x0FFF max, which 4096. Seems plenty for me. 
> 
> ice-9/psyntax.pp overflows it.  You need to make sure unusually big
> values don't result in garbage, even if the storage is optimized for
> sensible or usual cases.

Well, whatever. The 4096 limit has been there all the time.


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


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Kevin Ryde]

Han-Wen Nienhuys <hanwen@xs4all.nl> writes:
>
> why use a separate storage pool for srcprop objects?

At a guess, is it because that they're likely to never need freeing,
hence can be laid down in big blocks.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Neil Jerram]

Kevin Ryde <user42@zip.com.au> writes:

> Han-Wen Nienhuys <hanwen@xs4all.nl> writes:
>>
>> why use a separate storage pool for srcprop objects?
>
> At a guess, is it because that they're likely to never need freeing,
> hence can be laid down in big blocks.

I'd guess because setting up a srcprops is critical to start-up
performance, and a double cell doesn't have enough slots to store all
the common properties (filename, pos, copy) directly (as your change
makes clear).

So I'd expect --debug start up to go a little more slowly after your
change.  Did you make any measurements?

Also, did you find that the change saved a lot of memory for Lilypond?
It's not obvious to me why it should have done, just moving srcprops
from a specialized storage to the general cell heap.

Regards,
    Neil



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Han-Wen Nienhuys]

Neil Jerram escreveu:
> Kevin Ryde <user42@zip.com.au> writes:
> 
>> Han-Wen Nienhuys <hanwen@xs4all.nl> writes:
>>> why use a separate storage pool for srcprop objects?
>> At a guess, is it because that they're likely to never need freeing,
>> hence can be laid down in big blocks.
> 
> I'd guess because setting up a srcprops is critical to start-up
> performance, and a double cell doesn't have enough slots to store all
> the common properties (filename, pos, copy) directly (as your change
> makes clear).

All this guessing ...  I suspect it was done just because of poor design
and/or premature optimization.

on the factual side: 

1. the GUILE ends up with 1506 srcprops objects.

2. this is neglible compared to the 431777 total cells that
are allocated.

3. Due to sharing of the filename cons, memory usage is slightly more
than 4 SCMs per srcprop, down from 6 SCMs (2 for the smob cell, 4 for the
struct) 

I actually think it would be a good idea to generalize from double cells,
to cells containing any number between 3 and 8 SCM values.  This would 
be a better fit with some datatypes, and obviates the procustes
hacking to fit all the information inside some struct.

> It's not obvious to me why it should have done, just moving srcprops
> from a specialized storage to the general cell heap.

Because the code made me cringe. It's pointless to have specialized storage
for srcprops. it only makes the code more obtuse.

I you really want to know, ask Mikael Djurfeldt who added the bits 
in 20 aug 1996.

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


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Neil Jerram]

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

> Neil Jerram escreveu:
>> Kevin Ryde <user42@zip.com.au> writes:
>> 
>>> Han-Wen Nienhuys <hanwen@xs4all.nl> writes:
>>>> why use a separate storage pool for srcprop objects?
>>> At a guess, is it because that they're likely to never need freeing,
>>> hence can be laid down in big blocks.
>> 
>> I'd guess because setting up a srcprops is critical to start-up
>> performance, and a double cell doesn't have enough slots to store all
>> the common properties (filename, pos, copy) directly (as your change
>> makes clear).
>
> All this guessing ...  I suspect it was done just because of poor design
> and/or premature optimization.

Except you haven't given any objective reasons for why the design is
poor or the optimization premature.

> on the factual side: 
>
> 1. the GUILE ends up with 1506 srcprops objects.

Out of interest, in what scenario?

> 2. this is neglible compared to the 431777 total cells that
> are allocated.

(Which suggests to me that the decrease in memory from your change
wasn't that worthwhile.)

> 3. Due to sharing of the filename cons, memory usage is slightly more
> than 4 SCMs per srcprop, down from 6 SCMs (2 for the smob cell, 4 for the
> struct) 

Well that's nice, but only to be expected from throwing away a
performance/occupancy optimization.

> I actually think it would be a good idea to generalize from double cells,
> to cells containing any number between 3 and 8 SCM values.  This would 
> be a better fit with some datatypes, and obviates the procustes
> hacking to fit all the information inside some struct.

Maybe.  I think this would have to be motivated by looking at
particular cases where we get benefits from moving struct data into a
multiple cell.  I don't think the srcprops case is clearcut
(obviously), and I don't see anything wrong with the general approach
of indirecting to a struct.

> Because the code made me cringe. It's pointless to have specialized storage
> for srcprops. it only makes the code more obtuse.

I disagree.  I believe there was point to the code, and it was nowhere
near obtuse.

> I you really want to know, ask Mikael Djurfeldt who added the bits 
> in 20 aug 1996.

I don't think I need to.  Mikael was responsible for adding all
Guile's low-level debugging support, including this.  He took great
care to minimize the impact of this support on runtime performance -
which it seems to me that you are now throwing away because "the code
makes you cringe".

Regards,
     Neil



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Han-Wen Nienhuys]

Neil Jerram escreveu:
>>>>> why use a separate storage pool for srcprop objects?
>>>> At a guess, is it because that they're likely to never need freeing,
>>>> hence can be laid down in big blocks.
>>> I'd guess because setting up a srcprops is critical to start-up
>>> performance, and a double cell doesn't have enough slots to store all
>>> the common properties (filename, pos, copy) directly (as your change
>>> makes clear).
>> All this guessing ...  I suspect it was done just because of poor design
>> and/or premature optimization.
> 
> Except you haven't given any objective reasons for why the design is
> poor or the optimization premature.

Any deviation from the standard memory allocation scheme should have a
good reason, or -at least- a documented reason.

This design predates several revisions of the garbage collector, so I
suspect that whatever reason there was for this idea, is no longer
valid.

>> on the factual side: 
>>
>> 1. the GUILE ends up with 1506 srcprops objects.
> 
> Out of interest, in what scenario?

Startup (with --debug), which you brought up earlier. As I indicated
in an earlier message, I did not see any measurable change in startup
time.

>> 2. this is neglible compared to the 431777 total cells that
>> are allocated.
> 
> (Which suggests to me that the decrease in memory from your change
> wasn't that worthwhile.)

That was not the point of the change.

>> I actually think it would be a good idea to generalize from double cells,
>> to cells containing any number between 3 and 8 SCM values.  This would 
>> be a better fit with some datatypes, and obviates the procustes
>> hacking to fit all the information inside some struct.
> 
> Maybe.  I think this would have to be motivated by looking at
> particular cases where we get benefits from moving struct data into a
> multiple cell.  I don't think the srcprops case is clearcut
> (obviously), and I don't see anything wrong with the general approach
> of indirecting to a struct.

I don't mind either, but since the old code was using a struct with
its own 'optimized' memory management scheme, I assumed that an even
better scheme (due to lower memory use) could not be questioned.

>> Because the code made me cringe. It's pointless to have specialized storage
>> for srcprops. it only makes the code more obtuse.
> 
> I disagree.  I believe 

Can we have some measurable data?

Thanks,  

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



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Ludovic Courtès]

Hi,

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

> Startup (with --debug), which you brought up earlier. As I indicated
> in an earlier message, I did not see any measurable change in startup
> time.

I think startup time is hardly measurable, unless you have really old
hardware (and that makes statistical profiling unsuitable for the
startup process).  However, while not measurable, it _is_ noticeable.

Thanks,
Ludovic.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Han-Wen Nienhuys]

Ludovic Courtès escreveu:
>> Startup (with --debug), which you brought up earlier. As I indicated
>> in an earlier message, I did not see any measurable change in startup
>> time.
> 
> I think startup time is hardly measurable, unless you have really old
> hardware (and that makes statistical profiling unsuitable for the
> startup process).  However, while not measurable, it _is_ noticeable.

reading back my first message, I also tried loading all of ice-9; I couldn't 
detect changes with this test either.



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



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Mikael Djurfeldt]

2007/1/29, Han-Wen Nienhuys <hanwen@xs4all.nl>:
> Neil Jerram escreveu:
> > Kevin Ryde <user42@zip.com.au> writes:
> >
> >> Han-Wen Nienhuys <hanwen@xs4all.nl> writes:
> >>> why use a separate storage pool for srcprop objects?
> >> At a guess, is it because that they're likely to never need freeing,
> >> hence can be laid down in big blocks.
> >
> > I'd guess because setting up a srcprops is critical to start-up
> > performance, and a double cell doesn't have enough slots to store all
> > the common properties (filename, pos, copy) directly (as your change
> > makes clear).
>
> All this guessing ...  I suspect it was done just because of poor design
> and/or premature optimization.

While I have to admit that I was a novice programmer at the time I
wrote this code, and definitely didn't have enough experience to judge
what is a good design, I fail to see what is so bad about that code.
Also, please remember that things looked quite differently then.  For
example, there were no double cells.

Neil is quite right about the reasons for doing it like that.  As you
know, computers were at that time a lot slower.  Also, Guile, and
especially SCM which Guile was derived from, was far more "efficient",
so that small code changes wasn't drowned but had a noticeable impact.
 I would say the idea of storing a lot of information for every
s-expression in the code was at that time a bit outrageous, so I saw
it as very important to prove that this concept was in fact realistic.
 So I made sure that allocation size and time was optimized.  Another
aspect is that the breakpoint flag needed quick access in order to
test if such a scheme could be used for breakpoints instead of code
substitution (which might still be a better idea).

> on the factual side:
>
> 1. the GUILE ends up with 1506 srcprops objects.

Source properties have work also for large projects, so it's that kind
of situation we need to look at.  I think my own projects have reached
20000 objects or more.

> 2. this is neglible compared to the 431777 total cells that
> are allocated.

Yes, it could very well be the case that an extra effort is poorly
motivated by memory usage alone.

> 3. Due to sharing of the filename cons, memory usage is slightly more
> than 4 SCMs per srcprop, down from 6 SCMs (2 for the smob cell, 4 for the
> struct)

Hmm...  Your filename optimization doesn't really work, does it?  As
soon as someone sets a breakpoint, he gets it all over the place, or
did I miss something?

If I'm right, the 1996 "solution" was, at least, down to 6 SCM from 8
(since we didn't have double cells, while your solution is in fact
4+4=8.  But you could probably easily get it down to 6---not that it
matters much now.

> Because the code made me cringe. It's pointless to have specialized storage
> for srcprops. it only makes the code more obtuse.

If we considered implementing source properties *now*, I would
probably agree.  But the code is already there and I wonder if there
really is any gain of replacing it.  For example, the code you need to
add in order to do the filename optimization is hardly much more
maintainable than what we already had in there, and it is my guess
that whatever alternative code you propose, it won't be faster or
consume less memory.

Also, Neil should probably study the effects on his debugging code of
putting the breakpoint flag in the standard property list.  What
happens if the property list is used for other things so that it has
to be traversed for, for example, one step?

I guess you active guys should sort this out between yourselves.

Thanks for working on Guile,

Best regards,
M


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: freeing srcprops ?

[Han-Wen Nienhuys]

Mikael Djurfeldt escreveu:
>> 3. Due to sharing of the filename cons, memory usage is slightly more
>> than 4 SCMs per srcprop, down from 6 SCMs (2 for the smob cell, 4 for the
>> struct)
> 
> Hmm...  Your filename optimization doesn't really work, does it?  As
> soon as someone sets a breakpoint, he gets it all over the place, or
> did I miss something?

from what I could follow from the code, srcprops were mainly constructed in 
the reader. However, I would gladly remove that bit too, and simply go for 
cons-cell with struct pointer, but without the special memory pool.

Neil, is that acceptable?

> If we considered implementing source properties *now*, I would
> probably agree.  But the code is already there and I wonder if there
> really is any gain of replacing it.  

It's a question of philosophy. IMO, at any time, the source code of a project
should reflect how you would implement it "now"

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



_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel