develooper Front page | perl.perl5.porters | Postings from April 2006

Re: use arenas for NewOp's

Thread Previous
From:
Jim Cromie
Date:
April 2, 2006 09:33
Subject:
Re: use arenas for NewOp's
Message ID:
442FFCCB.2030608@gmail.com
Nicholas Clark wrote:
> Sorry for the delay in thinking about this. I had my head full with evil
> UTF-8 caching code, and now %^H
>
> On Sat, Mar 18, 2006 at 08:55:28PM -0500, Jim Cromie wrote:
>
>   
>> Im not entirely happy with the coupling between sv.c and op.c,
>> and between enum svtype and OP-slots.  it could probably be cleaned up 
>> some,
>> but the coupling is somewhat intrinsic, so Im deferring it pending 
>> suggestions.
>>     
>
> In my mind I was thinking more about having a discrete separate array for the
> op allocation, rather than merging it with the array used for SV bodies.
>
>   
OK - its a cleaner separation.   Will code it as PL_opbody_roots[].
The downside is that we cant use S_new_body (it indexes PL_body_roots[])
but we can have a parallel implementation for op-bodies.

>   
>> +#define slot_of_opclass(type) \
>> +	( #type == "BASEOP")		? SVt_LAST + OPt_BASEOP	\
>> +	: ( #type == "OP")		? SVt_LAST + OPt_BASEOP	\
>> +	: ( #type == "UNOP")		? SVt_LAST + OPt_UNOP	\
>> +	: ( #type == "BINOP")		? SVt_LAST + OPt_BINOP	\
>> +	: ( #type == "LOGOP")		? SVt_LAST + OPt_LOGOP	\
>> +	: ( #type == "LISTOP")		? SVt_LAST + OPt_LISTOP	\
>> +	: ( #type == "PMOP")		? SVt_LAST + OPt_PMOP	\
>> +	: ( #type == "SVOP")		? SVt_LAST + OPt_SVOP	\
>> +	: ( #type == "PADOP")		? SVt_LAST + OPt_PADOP		\
>> +	: ( #type == "PVOP_OR_SVOP")	? SVt_LAST + OPt_PVOP_OR_SVOP	\
>> +	: ( #type == "LOOP")		? SVt_LAST + OPt_LOOP		\
>> +	: ( #type == "COP")		? SVt_LAST + OPt_COP		\
>> +	: ( #type == "BASEOP_OR_UNOP")	? SVt_LAST + OPt_BASEOP_OR_UNOP	\
>> +	: ( #type == "FILESTATOP")	? SVt_LAST + OPt_FILESTATOP	\
>> +	: ( #type == "LOOPEXOP")	? SVt_LAST + OPt_LOOPEXOP	\
>> +	: ( -1 )	/* force error w bad svtype */
>>     
>
> That expands to
>
>     int slot = ( "type" == "BASEOP") ? SVt_LAST + (0) : ( "type" == "OP") ? SVt_LAST + (0) : ( "type" == "UNOP") ? SVt_LAST + (1) : ( "type" == "BINOP") ? SVt_LAST + (2) : ( "type" == "LOGOP") ? SVt_LAST + (3) : ( "type" == "LISTOP") ? SVt_LAST + (4) : ( "type" == "PMOP") ? SVt_LAST + (5) : ( "type" == "SVOP") ? SVt_LAST + (6) : ( "type" == "PADOP") ? SVt_LAST + (7) : ( "type" == "PVOP_OR_SVOP") ? SVt_LAST + (8) : ( "type" == "LOOP") ? SVt_LAST + (9) : ( "type" == "COP") ? SVt_LAST + (10) : ( "type" == "BASEOP_OR_UNOP") ? SVt_LAST + (11) : ( "type" == "FILESTATOP") ? SVt_LAST + (12) : ( "type" == "LOOPEXOP") ? SVt_LAST + (13) : ( -1 );
>
> which I don't think was really what you were thinking.
>
>   
ack.  what a mess that thinko was.

> I suspect that (type >> OCSHIFT) would give the right answer.
>
>   
thats the approach I used for FreeOp - works IFF you have a field to do 
that shift upon.
The trouble is that NewOp is passed a type (ex: BINOP), snf we need to 
map that to a slot.

I tried changing all the NewOp call-sites to pass OPt_*, where * is the 
type currently passed in,
but the macro to map OPt_* to a BINOP (for legacy equivalent) is just as 
hosed
as the mess above.

So Im punting on 2nd half for now, and just splitting arena-details out
from body-details.

>> -struct body_details {
>> -    U8 body_size;	/* Size to allocate  */
>> -    U8 copy;		/* Size of structure to copy (may be shorter)  */
>> -    U8 offset;
>> -    unsigned int type : 4;	    /* We have space for a sanity check.  */
>> -    unsigned int cant_upgrade : 1;  /* Cannot upgrade this type */
>> -    unsigned int zero_nv : 1;	    /* zero the NV when upgrading from this */
>> +struct arena_details {
>>      unsigned int arena : 1;	    /* Allocated from an arena */
>>      size_t arena_size;		    /* Size of arena to allocate */
>>  };
>>     
>
> I'm not sure I like this split done like this. I guess it's partly because I
> can see 33 bits in a structure and so I'm thinking "that's going to allocate
> 64", but also because there's an alternative conceptual split:
>
>
> struct body_details {
>     U8 body_size;	/* Size to allocate  */
>     U8 copy;		/* Size of structure to copy (may be shorter)  */
>     U8 offset;
>     unsigned int type : 4;	    /* We have space for a sanity check.  */
>     unsigned int cant_upgrade : 1;  /* Cannot upgrade this type */
>     unsigned int zero_nv : 1;	    /* zero the NV when upgrading from this */
>     unsigned int has_body : 1;	    /* Body needs allocating */
> }
> struct arena_details {
>     size_t arena_size;		    /* Size of arena to allocate */
> };
>
> where the flag bit is "do I need a body at all" and then the arena_details
> is consulted for the size, with a size of 0 meaning "use malloc"
>
>   
Ill have to admit here that I put the flag into arena-details to emphasize
that it isnt really necessary;  if arena_size == 0, the same "dont use 
arenas" results.

Ive adopted your field-name "has_body", but its still initialized with 
HASARENA, NOARENA.
And then theres body_size, which could say same thing; ie 0 means ! 
has_body.
I also note that body_details[SVt_RV].body_size == 0, though I dont 
think it was
intended in the sense discussed here.
Hmm.

> Except I'm realising as I type this that you can't swap to arenas from
> discrete body mallocs at runtime, because how do you know how to "free"
> any bodies? 
Oooh, tricksy.
This means that we cannot alter arena-sizes from 0 to non-0 or vice versa.
We can change the non-0 magnitude, as long as the boolean sense is 
preserved.

with this in mind, its arguably error-prone to have 2 separate ways of
changing arena/malloc choice: the has_body flag, and 
arena_details.arena_size

> And really, I feel that PVFM bodies should remain as malloc()
> because it seems that perl allocates just 2 on startup, most programs never
> use formats so no more will be needed, so all the arenas overhead seems
> wasteful.
>
>   
Ive shrunk the arena-size to 2 PVFMs, avoiding unused allocations.
The remaining overhead happens only once, so its negligible.
Im not fussy either way, and currently, PVFM's has_body == 0,
which sort of proves the point about too many ways to do it.


> And it looks like the existing code has the same issue, as the logic on
> whether to call del_body or my_safefree is based on general type data
> both before and after:
>
>   
>> @@ -5141,9 +5179,9 @@ Perl_sv_clear(pTHX_ register SV *sv)
>>      SvFLAGS(sv) &= SVf_BREAK;
>>      SvFLAGS(sv) |= SVTYPEMASK;
>>  
>> -    if (sv_type_details->arena) {
>> +    if (arenas_by_type[type].arena) {
>>  	del_body(((char *)SvANY(sv) + sv_type_details->offset),
>> -		 &PL_body_roots[type]);
>> +		 &PL_body_roots[MAP_SIZE(type)]);
>>      }
>>      else if (sv_type_details->body_size) {
>>  	my_safefree(SvANY(sv));
>>     
>
> I'm still not sure what the best way is.
>
>
>
>
> Searching for code conditional on PL_OP_SLAB_ALLOC I find this in op.c:
>
> #ifdef PL_OP_SLAB_ALLOC
>     {
> 	LOOP *tmp;
> 	NewOp(1234,tmp,1,LOOP);
> 	Copy(loop,tmp,1,LISTOP);
> 	FreeOp(loop);
> 	loop = tmp;
>     }
> #else
>     Renew(loop, 1, LOOP);
> #endif
>
> so I think that arenas for ops are doomed to crash if that one isn't changed.
>
>   
Yes - in the patch, I had:

-#ifdef PL_OP_SLAB_ALLOC
+#if defined(PL_OP_SLAB_ALLOC) || defined (OPARENAS)

which should work - at least its not obviously broken




> Secondly, the op allocation code seems to be on the basis of these C types:
>
>     typedef struct op OP;
>     typedef struct cop COP;
>     typedef struct unop UNOP;
>     typedef struct binop BINOP;
>     typedef struct listop LISTOP;
>     typedef struct logop LOGOP;
>     typedef struct pmop PMOP;
>     typedef struct svop SVOP;
>     typedef struct padop PADOP;
>     typedef struct pvop PVOP;
>     typedef struct loop LOOP;
>
> rather than the numeric types





>  laid down by
>
>
> so I'm starting to think that getting this to work depends on making the
> newop macro map those 10 C types to slots, rather than the 13 classes.
>
>   
this is subtle and tricky.

opclass is whats available in the OP itself, it seems the obvious thing 
to key upon
when returning the OP-body back to its free-list. 
Presuming that the 13 opclasses map down to 10 types (that theyre built 
from)
this isnt a show-stopper.

But Im still stuck on how to create the mappings that are needed.
hints welcome (hopefully >1 person reads this far)

> Nicholas Clark
>
>   

thanks
jimc

Thread Previous


nntp.perl.org: Perl Programming lists via nntp and http.
Comments to Ask Bjørn Hansen at ask@perl.org | Group listing | About