[poky] About the operator "??="

[Richard Purdie]

On Thu, 2010-12-09 at 15:59 +0800, Xu, Dongxiao wrote:
> Tian, Kevin wrote:
> >> From: Xu, Dongxiao
> >> Sent: Thursday, December 09, 2010 11:33 AM
> >> During the file parsing process, the getVar is called
> >> "1435156/1148654" 
> >> times (actually I am still confused what "/" means...), and among
> >> them, 322121 times of getVar call return "None", thus about 1/3 or
> >> 1/4 of the total calls. 

Ok, that is a pretty significant portion of the time.

> >> I tried to use the following patch to have a test. Also I paste a
> >> profile log. 
> >> From the result we can see it has over 15% performance gain. (from
> >> 39.3 
> >> secs to 33.7 secs).

So despite that, it still significantly helps performance. Could you
also give some raw performance numbers? By this I mean make the timing
runs with profiling turned off so work out the time it takes before and
after the patch with the profiling off so the numbers are "real" with
"time bitbake xxx"?

It always pays just to sanity check and this gives the "real"
performance gain.

> >> In usermanual, the "??=" definition is:
> >> 
> >> Setting a default value (??=)
> >> 	A ??= "somevalue"
> >> 	A ??= "someothervalue"
> >> If A is set before the above, it will retain that value.  If A is
> >> unset 
> >> prior to the above, A will be set to someothervalue.  This is a lazy
> >> version of ?=, in that the assignment does not occur until the end of
> >> the parsing process, so that the last, rather than the first, ??=
> >> assignment to a given variable will be used. 
> >> 
> >> Our patch logic isn't strickly following the "??=" definition, since
> >> the assignment doesn't occurs in end of parsing process...
> >> Except this one, I think other behaviors for ??= do not change.
> > 
> > I think it is, as long as a getVar on a variable happens after all
> > the evaluations of that variable which I think should be true or else
> > even current design has problem. In that way when evaluating variable
> > assignments, you keep 'defaultvalue' updated until the last "??="
> > assignment. Then later a getVar() on that variable happens which then
> > you just return the right defaultvalue. :-)    
> 
> Agree.

I also agree, I think we can adjust the definition of how this is
handled slightly. There is also a change in behaviour as now, when one
of these variables is looked up, it will get the default value rather
than None if finalise() hasn't been called. I don't think this is much
of an issue though.

> >> --- a/bitbake/lib/bb/parse/ast.py
> >> +++ b/bitbake/lib/bb/parse/ast.py
> >> @@ -109,10 +109,8 @@ class DataNode(AstNode):
> >>         if 'flag' in groupd and groupd['flag'] != None:
> >>             bb.data.setVarFlag(key, groupd['flag'], val, data)
> >>         elif groupd["lazyques"]:
> >> -            assigned = bb.data.getVar("__lazy_assigned", data) or []
> >> -            assigned.append(key)
> >> -            bb.data.setVar("__lazy_assigned", assigned, data)
> >>             bb.data.setVarFlag(key, "defaultval", val, data)
> >> +            bb.data.setVar(key, None, data)
> > 
> > This I think is incorrect. You actually wiped out previous assignment
> > before ??=, and thus end up to always have default value favored
> > unless there're other direct assignments after ??= evaluation. Just
> > touch the flag should be enough.   
> 
> Ah, yes, I need to check whether the value has been setVar before, see following patch. 
> 
> This two lines are necessary in the following patch
> 
> +            if bb.data.getVarFlag(key, "content", data) is None:
> +                bb.data.setVar(key, None, data)
> 
> because setVar will add the key into override list, which could not be
> achieved by just calling "setVarFlag". For the later formal patch, I
> will add comments on the two lines.

This is a little worrying and I think we have insight into a new bug
here which is that if you just have a variable:

foo_OVERA[bar] = "x"

and foo is never set as a variable directly, the override is never
expanded.

There is a strong argument here to make setVar(...) a special case of
setVarFlag(..., "content") although I worry about the performance
implications.

Chris: Any opinion?

Cheers,

Richard