[PATCH] Turning `scm_is_pair ()' into a macro

[PATCH] Turning `scm_is_pair ()' into a macro

[Ludovic Courtès]

Hi,

I believe it *really* makes sense to turn `scm_is_pair ()' into a macro.

Before:

   %   cumulative   self              self     total           
  time   seconds   seconds    calls   s/call   s/call  name    
  14.74     17.99    17.99 25918076     0.00     0.00  scm_is_pair
  13.72     34.73    16.74   104054     0.00     0.00  deval
  13.27     50.92    16.19    71854     0.00     0.00  scm_i_sweep_card
  13.20     67.03    16.11  3359204     0.00     0.00  scm_gc_mark_dependencies
  ...

After:

   %   cumulative   self              self     total           
  time   seconds   seconds    calls   s/call   s/call  name    
  16.11     17.92    17.92   104054     0.00     0.00  deval
  15.69     35.37    17.45    71854     0.00     0.00  scm_i_sweep_card
  14.77     51.80    16.43  3360490     0.00     0.00  scm_gc_mark_dependencies
  ...

Thanks,
Ludovic.

PS: You might have noticed that GC takes more than twice as long as
    evaluation on this example.  This is the subject of fascinating
    ongoing work.  ;-)


2005-12-14  Ludovic Courtès  <ludovic.courtes@laas.fr>

	* pairs.h (scm_is_pair): Made a macro rather than a function.

	* pairs.c (scm_is_pair): Only defined for binary compatibility.


--- orig/libguile/pairs.c
+++ mod/libguile/pairs.c
@@ -78,11 +78,6 @@
 }
 #undef FUNC_NAME
 
-int
-scm_is_pair (SCM x)
-{
-  return SCM_I_CONSP (x);
-}
 
 SCM
 scm_car (SCM pair)
@@ -203,6 +198,14 @@
 #include "libguile/pairs.x"
 }
 
+/* FIXME: This is temporarily kept for binary compatilibity and should
+   eventually be removed.  */
+#undef scm_is_pair
+int
+scm_is_pair (SCM x)
+{
+  return SCM_I_CONSP (x);
+}
 
 /*
   Local Variables:


--- orig/libguile/pairs.h
+++ mod/libguile/pairs.h
@@ -78,7 +78,7 @@
 SCM_API void scm_error_pair_access (SCM);
 #endif
 
-SCM_API int scm_is_pair (SCM x);
+#define scm_is_pair(__obj)  (SCM_I_CONSP (__obj))
 
 SCM_API SCM scm_cons (SCM x, SCM y);
 SCM_API SCM scm_cons2 (SCM w, SCM x, SCM y);



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

Re: [PATCH] Turning `scm_is_pair ()' into a macro

[Kevin Ryde]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:
>
> 	* pairs.c (scm_is_pair): Only defined for binary compatibility.

scm_is_pair is new in the cvs head, no need for a compatibility func.


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

Re: [PATCH] Turning `scm_is_pair ()' into a macro

[Han-Wen Nienhuys]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

In article <87wti7pwie.fsf@laas.fr>,
Ludovic Courtès <ludovic.courtes@laas.fr> wrote:
>Hi,
>
>I believe it *really* makes sense to turn `scm_is_pair ()' into a macro.
>
>--- orig/libguile/pairs.h
>+++ mod/libguile/pairs.h
>@@ -78,7 +78,7 @@
> SCM_API void scm_error_pair_access (SCM);
> #endif
> 
>-SCM_API int scm_is_pair (SCM x);
>+#define scm_is_pair(__obj)  (SCM_I_CONSP (__obj))

Why not use an inline function? That's cleaner, and you get the
backward compat for free.




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

Re: [PATCH] Turning `scm_is_pair ()' into a macro

[Marius Vollmer]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:

> I believe it *really* makes sense to turn `scm_is_pair ()' into a macro.

Yep.  But what about an inline function?  There is some machinery in
inline.h for this and we already use it for scm_cell, for example.

> +#define scm_is_pair(__obj)  (SCM_I_CONSP (__obj))

SCM_I_CONSP is not a safe macro, it expands its argument twice so we
shouldn't use it just like this.  (What is the point of the double
underscores?)

(It is true that I didn't care about performance at all when
introducing scm_is_pair and your approach is the exactly right one:
profile and identify the real bottlenecks.)

-- 
GPG: D5D4E405 - 2F9B BCCC 8527 692A 04E3  331E FAF8 226A D5D4 E405


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

Re: [PATCH] Turning `scm_is_pair ()' into a macro

[Ludovic Courtès]

Hi,

Marius Vollmer <mvo@zagadka.de> writes:

> Yep.  But what about an inline function?  There is some machinery in
> inline.h for this and we already use it for scm_cell, for example.

I'm all in favor of inline functions.  But since we want to support
compilers that don't support inlining, we have to use the whole
machinery that's in `inline.h'.  That machinery is only used in
`inline.h', while we would like `scm_is_pair ()' to be defined in
`pairs.h'.

IOW, making the inlining machinery easily usable is not that easy, and,
well, I'm lazy too.  ;-)  Do you have a suggestion for this?

Since `scm_is_pair ()' was already in 1.7.2, maybe it'd make sense to
keep a non-inlined version of it.  Maybe not (e.g., if we are to change
library version numbers).

BTW, below is a related fix that I forgot to post earlier.

Thanks,
Ludovic.


2005-12-15  Ludovic Courtès  <ludovic.courtes@laas.fr>

	* socket.c: Include "libguile/pairs.h".


--- orig/libguile/socket.c
+++ mod/libguile/socket.c
@@ -35,6 +35,7 @@
 
 #include "libguile/validate.h"
 #include "libguile/socket.h"
+#include "libguile/pairs.h"
 
 #ifdef __MINGW32__
 #include "win32-socket.h"



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

Re: [PATCH] Turning `scm_is_pair ()' into a macro

[Han-Wen Nienhuys]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 390 bytes --]

In article <87fyouitqo.fsf@laas.fr>,
Ludovic Courtès <ludovic.courtes@laas.fr> wrote:
>IOW, making the inlining machinery easily usable is not that easy, and,
>well, I'm lazy too.  ;-)  Do you have a suggestion for this?

Yes, Just Do It :-)




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

Re: [PATCH] Turning `scm_is_pair ()' into a macro

[Ludovic Courtès]

hanwen@byrd.xs4all.nl (Han-Wen Nienhuys) writes:

> Yes, Just Do It :-)

Since, I consider compilers that don't support inlining unimportant, I'd
happily live without the `inline.c' stuff.  I.e., I'd put this in
`pairs.h':

  static SCM_C_INLINE int
  scm_is_pair (...)

With compilers not supporting inlining, this would lead to the creation
of several instances of the function.  But who cares?

Marius?

Thanks,
Ludovic.


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

[PATCH] Inlining `scm_is_pair ()'

[Ludovic Courtès]

Hi,

ludovic.courtes@laas.fr (Ludovic Courtès) writes:

> Since, I consider compilers that don't support inlining unimportant, I'd
> happily live without the `inline.c' stuff.  I.e., I'd put this in
> `pairs.h':
>
>   static SCM_C_INLINE int
>   scm_is_pair (...)
>
> With compilers not supporting inlining, this would lead to the creation
> of several instances of the function.  But who cares?

The following patch makes `scm_is_pair ()' an inline --- the macro was
indeed a bad idea because there are places (e.g., async.c:208) where its
argument is an assignment.

I'm not sure that's the only reason, but I'm now unable to compile Guile
with `-O0': when doing so, I get a "stack overflow" error when trying to
run the REPL.  Perhaps we should also declare the function with
`__attribute__ ((always_inline))'?

What about non-inlining compilers, are there still a lot of them?  I'd
suspect GCC is now used 40% of the time on proprietary Unices;
hopefully, the proprietary compilers used the rest of the time do
support the `inline' keyword [0], some of them even support C99 [1].
OTOH, it seems that Compaq's only supports `inline' as a pragma [2].

How about MSVC?

Thanks,
Ludovic.

[0] http://docs.hp.com/en/5990-8153/ch10s05.html
[1] http://developers.sun.com/prodtech/cc/documentation/ss9_docs/mr/READMEs/c.html
[2] http://h30097.www3.hp.com/dtk/Compaq_C_Compiler/doc/lrm/DOCU0030.HTM


2006-01-24  Ludovic Courtès  <ludovic.courtes@laas.fr>

	* pairs.c (scm_is_pair): Removed.

	* pairs.h (scm_is_pair): Defined as `static inline'.

	* socket.c: Include "pairs.h".


--- orig/libguile/pairs.c
+++ mod/libguile/pairs.c
@@ -78,11 +78,6 @@
 }
 #undef FUNC_NAME
 
-int
-scm_is_pair (SCM x)
-{
-  return SCM_I_CONSP (x);
-}
 
 SCM
 scm_car (SCM pair)


--- orig/libguile/pairs.h
+++ mod/libguile/pairs.h
@@ -23,6 +23,7 @@
 \f
 
 #include "libguile/__scm.h"
+#include "libguile/gc.h"  /* SCM_CELL_TYPE */
 
 \f
 
@@ -78,7 +79,12 @@
 SCM_API void scm_error_pair_access (SCM);
 #endif
 
-SCM_API int scm_is_pair (SCM x);
+static SCM_C_INLINE int
+scm_is_pair (SCM obj)
+{
+  return (SCM_I_CONSP (obj));
+}
+
 
 SCM_API SCM scm_cons (SCM x, SCM y);
 SCM_API SCM scm_cons2 (SCM w, SCM x, SCM y);


--- orig/libguile/socket.c
+++ mod/libguile/socket.c
@@ -35,6 +35,7 @@
 
 #include "libguile/validate.h"
 #include "libguile/socket.h"
+#include "libguile/pairs.h"
 
 #ifdef __MINGW32__
 #include "win32-socket.h"



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

Re: [PATCH] Inlining `scm_is_pair ()'

[Kevin Ryde]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:
>
> The following patch makes `scm_is_pair ()' an inline --- the macro was
> indeed a bad idea because there are places (e.g., async.c:208) where its
> argument is an assignment.

Which is probably not really in line with gnu coding standards
(standards.info end of "Syntactic Conventions" node).  But I guess any
application might do it.

> I'm not sure that's the only reason, but I'm now unable to compile Guile
> with `-O0': when doing so, I get a "stack overflow" error when trying to
> run the REPL.  Perhaps we should also declare the function with
> `__attribute__ ((always_inline))'?

That was ceval/deval() local variables on gcc 4 last time I hit that.
-O1 may be better, or just on eval.c at least.

> +static SCM_C_INLINE int

I suspect extern inline is what you want, static inline might end up
with a copy of the code in every file, under -O0 at least.  But extern
inline is even more of a gcc-ism, if you're worried about the
commercial compilers.


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

Re: [PATCH] Inlining `scm_is_pair ()'

[Ludovic Courtès]

Hi,

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

> Which is probably not really in line with gnu coding standards
> (standards.info end of "Syntactic Conventions" node).  But I guess any
> application might do it.

Certainly not the only thing that's not GCS-compatible in Guile...  ;-)

> That was ceval/deval() local variables on gcc 4 last time I hit that.
> -O1 may be better, or just on eval.c at least.

Right, I don't have the "stack overflow" with `-O1', fortunately.

>> +static SCM_C_INLINE int
>
> I suspect extern inline is what you want, static inline might end up
> with a copy of the code in every file, under -O0 at least.  But extern
> inline is even more of a gcc-ism, if you're worried about the
> commercial compilers.

With `-O0', no inlining takes place, unless the `always_inline'
attribute was specified.

`extern inline' is a indeed a GCCism.  It might be better since, as you
said, it would allow a single definition to be used in the case where
inlining doesn't take place.  Quoting the GCC manual:

   If you specify both `inline' and `extern' in the function definition,
  then the definition is used only for inlining.  In no case is the
  function compiled on its own, not even if you refer to its address
  explicitly.  Such an address becomes an external reference, as if you
  had only declared the function, and had not defined it.

OTOH, the code of this function is sufficiently small so that code
duplication shouldn't be a concern---if it was, we wouldn't be inlining
in the first place.

So I think it'd make sense to apply it as is.  We'll have to ask people
who work with proprietary compilers to voice out if there's something
wrong anyway.

Thanks,
Ludovic.


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

Re: [PATCH] Inlining `scm_is_pair ()'

[Kevin Ryde]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:
>
> We'll have to ask people who work with proprietary compilers to
> voice out if there's something wrong anyway.

I doubt you'll ever get any feedback like that.  Making something that
takes advantage of gcc but does no harm in vanilla c89 or c99 is in a
sense the best of both worlds.


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

Re: [PATCH] Inlining `scm_is_pair ()'

[Ludovic Courtès]

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

> I doubt you'll ever get any feedback like that.  Making something that
> takes advantage of gcc but does no harm in vanilla c89 or c99 is in a
> sense the best of both worlds.

C99 defines `inline', so nothing wrong problem with that.  As for C89,
it would do no harm with it: the function's code will simply be
duplicated in all files that use it, except that it will not be
inlined.  At worst, some compilers may raise warnings about the function
being declared as `static' but unused.

To me, this looks acceptable.  If anyone has better suggestions, that's
cool.  But please, don't leave `scm_is_pair ()' as is.

Thanks,
Ludovic.


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

Re: [PATCH] Inlining `scm_is_pair ()'

[Marius Vollmer]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:

> The following patch makes `scm_is_pair ()' an inline [...]

I just put scm_is_pair into inline.h, using our existing inlining
machinery that should be portable enough.  Ok?

> I'm not sure that's the only reason, but I'm now unable to compile Guile
> with `-O0': when doing so, I get a "stack overflow" error when trying to
> run the REPL.

This happens to me with and without scm_is_pair being an inline.  Our
default stack limit just seems to be too tight.  We could arbitarily
raise it from 20000 to, say, 30000.

-- 
GPG: D5D4E405 - 2F9B BCCC 8527 692A 04E3  331E FAF8 226A D5D4 E405


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

Re: [PATCH] Inlining `scm_is_pair ()'

[Ludovic Courtès]

Hi,

Marius Vollmer <mvo@zagadka.de> writes:

> I just put scm_is_pair into inline.h, using our existing inlining
> machinery that should be portable enough.  Ok?

Ok, thanks!  But (there's always a `but' ;-)) it feels somewhat wrong to
put it in `inline.h' rather than in `pairs.h' just because it's inline.

> This happens to me with and without scm_is_pair being an inline.  Our
> default stack limit just seems to be too tight.  We could arbitarily
> raise it from 20000 to, say, 30000.

Right, maybe we should do it.

Thanks,
Ludovic.


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

Re: [PATCH] Inlining `scm_is_pair ()'

[Kevin Ryde]

Marius Vollmer <mvo@zagadka.de> writes:
>
> I just put scm_is_pair into inline.h, using our existing inlining
> machinery that should be portable enough.

Maybe scm_is_eq too (picking up scm_is_true and scm_is_false at the
same time).


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

Re: [PATCH] Inlining `scm_is_pair ()'

[Marius Vollmer]

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

> Marius Vollmer <mvo@zagadka.de> writes:
>>
>> I just put scm_is_pair into inline.h, using our existing inlining
>> machinery that should be portable enough.
>
> Maybe scm_is_eq too (picking up scm_is_true and scm_is_false at the
> same time).

scm_is_eq is a macro (becase it doesn't need to evaluate its arguments
twice).

-- 
GPG: D5D4E405 - 2F9B BCCC 8527 692A 04E3  331E FAF8 226A D5D4 E405


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

Re: [PATCH] Turning `scm_is_pair ()' into a macro

[Kevin Ryde]

Marius Vollmer <mvo@zagadka.de> writes:
>
> Yep.  But what about an inline function?  There is some machinery in
> inline.h for this and we already use it for scm_cell, for example.

Was some of the change to scm_is_whatever meant to keep the
representation of an SCM value out of application binaries?
I lost track of what it was supposed to be doing. :)


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

Re: [PATCH] Turning `scm_is_pair ()' into a macro

[Marius Vollmer]

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

> Was some of the change to scm_is_whatever meant to keep the
> representation of an SCM value out of application binaries?

No, it was about having a clean distinction between functions that
return SCM booleans (scm_whatever_p) and those that return C booleans
(scm_is_whatever).

-- 
GPG: D5D4E405 - 2F9B BCCC 8527 692A 04E3  331E FAF8 226A D5D4 E405


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