Re: [pygccxml-development] Another performance tweak

[Roman Y. <rom...@gm...>]

On 8/27/06, Allen Bierbaum <al...@vr...> wrote:
> I have included comments below.  Before that though...
>
> I have found a few more optimizations:
>
> - Making create_identifier return identity adds about 15% to performance
> - Adding a cache to module_builder that caches the global namespace tree
> after parsing, joining, and running the query optimizer.  This boosts
> performance by another 40%.  (both by saving more startup and making the
> query optimization initialization less costly so it can be used)

Can you  explain relationship between it and declarations cache in pygccxml?

> So now I have my run time down to aroung 59 seconds.  Although I would
> really like to make it go even faster I think taking the running time
> down from the original 12 minutes to 1 minute now is a good enough
> improvement to make it so I can use py++ much easier in normal development.

:-)

> Roman:  Do you want a patch for module_builder caching or do you just
> want me to commit it?  (all the changes are in the __init__ method and
> are very clear so you can see what is in there and change it if you prefer)

Please commit them, but still explain the relationship :-)

> Now on to the comments....
>
> Roman Yakovenko wrote:
>
> > On 8/26/06, Allen Bierbaum <al...@vr...> wrote:
> >
> >> >> - Caching the results of type_traits.remove_alias improved
> >> performance
> >> >> by 20%
> >> >
> >> >
> >> > Yes, this algorithms is used every where. Please don't commit this
> >> > change, I'd like
> >> > to think about it. I see next problem here: there are a lot of types,
> >> > so the cache
> >> > memory size could go out of control. Can you find out how many memory
> >> > Py++ takes with\without the cache?
> >>
> >> I can try to take a look at this.  I don't have an easy way to do it
> >> right now but I will try later.
> >
> >
> > I am think that I don't want to introduce cache of results of
> > type_traits.
> > I don't feel comfortable with them. One of the reasons is that it is
> > not that easy to
> > disable them.
>
> Maybe I am missing something, but why can't we control them by
> introducing a flag in type_traits like "caching_enabled" or something
> and then just testing that whenever the method is called.
>
> It would be a shame not to optimize this method when it gives a 20%
> performance boost.

You are right, I don't have good reason. I will commit the patch. By
the way it was
a tricky one. One of my test failed. It was very difficult ( few hours
with debugger )
to find the problem. That is exactly the reason why I hate cache, but
what can I do
if you like it :-) ?

> Are you suggesting I add this as a new method and change the code that
> calls it?

No. I just committed the results.

> In my own local copy I have replaced "created_identifier" with an
> identify function and am getting very good results.

Why do you have goodies? For a time being please put it in a goodies.

> Right now I am not quite ready to update to your changes.  I have some
> concerns about the complexity and performance of the way you implemented
> this.

Do you have the numbers? I did not read the benchmark results. I will
do this later.
You are right, I introduced one more "get attribute". I have 1 good reason
for this maintainability. I'd like to keep all "cached" values in single place.
Thus, I don't have to scan sources for "what I am caching" and more over
it is very easy to create documentation to it. I am sure you will not see this
 "get attribute" in the benchmark


> - Why is there an "algorithms_cache_t" object as the base class to
> "declaration_algs_cache_t"?  The split with a base class doesn't seem to
> serve much of a purpose.

Enable\disable functionality. But I think it is useless now. The
better idea is to
introduce type_algs_cache_t class with class variable , that will control all
instances of type_algs_cache_t.

> - What is the performance implication of using "properties"?  I know you
> like to use them in your code but how do they affect performance?

I tried with and without them and I did not see the difference. May be you will
see. If you do see the difference, we can leave set_* methods and remove
property

> - Handling of enabled flag.  I think the handling of the enabled flag
> should be done in the "set" method instead of the get method.  As it
> stands with your change, our optimized path will require two if tests
> (one in the local method to test for None, and one in the get method to
> test enabled).  If we moved the enabled test to the set method we would
> only pay the cost of that if when we have optimizations diabled.

You are right. Fixed.

>
> > I left some interesting problem to you: it is possible to optimize
> > declaration_path
> > algorithm even more. In most cases its complexity could be O(1). This
> > could be done
> > by saving intermediate results. Another way to say it: to re-use
> > parent declaration path
> > cache.

What about this?

> > Can you publish top N lines of your benchmark result?
> >
> Sure.  Give me a bit to rerun it.  I can probably just send you a
> compress hotspot file so you can see the entire details.

Thanks, I got it and will take a look later, okay?


-- 
Roman Yakovenko
C++ Python language binding
http://www.language-binding.net/