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

[Jörg F . Wittenberger]

Am 27.02.2016 um 14:46 schrieb 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.


"make check" just passed for modifications like this

#if USE_OLD_AV  // NOT APPLICABLE
...
#elsif USE_FIXED_DFLT  // THIS IS WHAT IS ACTUALLY EXPANDED
// TBD: runtime internal
#define C_argvector_flush()        (C_default_argvector_value = NULL)
#define C_argvector_size(av) (C_default_argvector_value == av ? /*FIXME
TEMPORARY_STACK_SIZE should be in runtime.c where it is known */ 4096 : 0)

#define C_force_allocate_fresh_argvector(avl) ( C_demand(avl) ?
(C_default_argvector_value = C_alloc(avl)) : (C_argvector_flush(),
C_temporary_stack))

// TDB: leave only these exported #defines
#define C_allocate_fresh_argvector(avl) ( /*FIXME should we
assert(avl<=limit)*/ C_default_argvector_value != NULL ?
C_default_argvector_value : C_force_allocate_fresh_argvector(avl))

// TBD: try this, may be faster: #define C_allocate_argvector(c, av,
avl) ( (c >= avl) ? av : C_force_allocate_fresh_argvector(avl))
// TBD: try assert(C_default_argvector_value != NULL) here and never
look back if this works. (deferred)
#define C_allocate_argvector(c, av, avl) ( (C_default_argvector_value !=
NULL) ? C_default_argvector_value : C_force_allocate_fresh_argvector(avl))

#else
... the length tagged argvector version here
#endif


Let me call it a day now.

Tomorrow is the dog's day.  More testing, benchmark and a patch not
before Monday.

Cheers

/Jörg


> 
> 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
> 
> _______________________________________________
> Chicken-hackers mailing list
> address@hidden
> https://lists.nongnu.org/mailman/listinfo/chicken-hackers
>