Re: [Chicken-hackers] Release blocker? Stack checks completely FUBAR? Or am I just paranoid?

[Jörg F . Wittenberger]

Am 27.02.2016 um 13:09 schrieb Jörg F. Wittenberger:
> Am 27.02.2016 um 12:25 schrieb Jörg F. Wittenberger:
>> Hi folks,
>>
>> if you really consider anything to be done to the argvector handling
>> before the next release
> 
> ...
> 
> I wonder: why not malloc exactly one argvector of TEMPORARY_STACK_SIZE
> word and drop all the checking?
> 
> Then either the current av vector is the one passed in, then we can
> safely reuse it.  If it's not we re-use the global anyway.

Looking at those options I wonder if there is not an even better option.

This is the relevant change in my patch:

#if USE_OLD_AV
#define C_allocate_fresh_argvector(n) C_alloc(n)
#define C_allocate_argvector(c, av, avl) ( (c >= avl) ? av :
C_force_allocate_fresh_argvector(avl))
#define C_argvector_flush() /* does nothing */

Those two are the macros doing the argvector handling.  Here mapping
back to equivalent code of what master would do.  Only those are called
from runtime.c and the C backend.  Well, not exactly, there is a
C_kontinue_av, which is a av C_kontinue. Trivial.  However runtime.c
already uses it (this is where the performance gain came in).  But it's
#defined back to C_kontinue depending on USE_OLD_AV anyways.

#else

This is the implementation of the length tagged argvector.

#define C_argvector_reuse_dflt(n)  ((C_default_argvector_value != NULL)
&& (C_default_argvector_value[0] >= (n)))
#define C_argvector_flush()        (C_default_argvector_value = NULL)

This would have to be changed to possibly reset the
C_default_argvector_value and return the temporary_stack.

#define C_force_allocate_fresh_argvector(n) ((C_default_argvector_value
= C_alloc((n)+1)), *C_default_argvector_value=(n),
C_default_argvector_value+1)

#define C_allocate_fresh_argvector(avl) (C_argvector_reuse_dflt(avl) ?
C_default_argvector_value+1 : C_force_allocate_fresh_argvector(avl))
#define C_argvector_size(av) (av[-1])
#define C_allocate_argvector(c, av, avl) ((((c) >= (avl)) ||
(C_argvector_size(av) >= (avl))) ? av :
C_force_allocate_fresh_argvector(avl))
#endif

If, instead, we would only ever put a pointer to a stack allocated
argument vector large enough for the apply count into the
C_default_argvector_value.  Then we could forgo the whole length tagging
and checking.  Even against "c" we may or may not want to check
(profiling will show).

Something like this should work:

#define C_allocate_fresh_argvector(avl) ( C_default_argvector_value !=
NULL ? C_default_argvector_value : C_demand(avl) ?
C_default_argvector_value = C_alloc(TEMPORARY_STACK_SIZE) :
C_default_argvector_value = NULL, temporary_stack)

Except for the TEMPORARY_STACK_SIZE, which is not available to macros
and that's a good thing.  However as this not in no way critical code,
we might want to call into runtime.c like
C_a_i_allocate_fresh_argvector(naming convections?) which would respect
runtime options etc.

As long as no code flushes the C_default_argvector value, that one will
be reused anyway.  Flushing it does the C_reclaim.  Last resort is the
temporary stack.  Passing as argument vector we still can anything.

> The downside: now we have one more area to scan in the garbage
> collector.  (That's why I preferred to stack allocated one so far).

Upside: not additional code in the gc.

> However reading the related code I get the feeling that we even could
> simply use the temporary_stack as the argvector.  However I'm not sure
> about that one.
> 
> Just thoughts.

Comments?


Cheers

/Jörg