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

[Jörg F . Wittenberger]

There is an number of arguments limit documented in "Deviations from the
standard".  (BTW: I guess the 1000 claimed there is outdated.)

But is this limit checked anywhere?  My reading of C_apply makes be
wonder.  Am I missing a check elsewhere?

Otherwise I'd add it with the patch.

BTW: currently a make check completed here.  This one re-uses a single,
stack allocated argvector of TEMPORARY_STACK_SIZE whenever the current
code would do C_alloc or av2[N].  Notable exception C_context_switch.
Interestingly: while this is the simplest code one could use, the length
tagged argvector is consistently about 2% faster here on all tests so far.

Cheers

/Jörg

Am 27.02.2016 um 20:58 schrieb 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?