Re: [Libreoffice] [PATCH] 1/4 tools/rtti.hxx cleaning

On Mon, Dec 19, 2011 at 3:57 PM, Michael Meeks <michael.meeks@suse.com>wrote:

> Hi Rafael,
>
> On Mon, 2011-12-19 at 13:03 -0430, Rafael Dominguez wrote:
> > Just some cleanup of tools/rtti.hxx macros for c++ RTTI.
>
>         Gosh :-) you are brave. I suppose there is some residual
> performance
> concern about this sort of thing; then again I think that many of our
> SfxItemSet items are rather shallowly inherited - just a couple of jumps
> to SfxFooItem etc. - so perhaps it won't be so bad. The SfxItemSet
> grab-bag is used in some pretty inner of inner loops JFYI so ... lets
> see.

Well yeah theres a penalty for using dynamic_cast but dunno how much will
affect performance since theres alot of calls, but im pretty sure some code
can be refactored to avoid casting.

> >  Theres a bunch of patchs but they are very straightfoward.
> > In the future can i push the patch myself or should i continue sending
> > them to the ml for review??
>
>         If we're resolved to do this thing; which - I suspect we should,
> then -
> if you are 100% confident, then I'm not sure these would need review. I
> wonder if there is an easy way (by leaving the existing macros in-place
> but changing their impl. to use dynamic_cast) whether we could do some
> profiling of: medium sized word document (with diverse styles &
> formatting) load, same for medium sized spreadsheet, and large PPT -
> before and after. [ the best way to do that is to run before & after in
> callgrind the way to do that is:
Well probably we can change the implementation of the PTR_CAST, HAS_BASE
and IS_TYPE macros, but for the other ones
we cant. i will see if i can get some benchmarks.


> export OOO_EXIT_POST_STARTUP=1
> export OOO_DISABLE_RECOVERY=1
> valgrind --tool=callgrind --simulate-cache=yes --dump-instr=yes
> ./soffice.bin -writer --splash-pipe=0 <test-file.ods>
>
>        You can then leave it un-attended & get a nice number out.
>
>        Beyond the malingering performance concern, I'm all for this :-)
>
>        As a minor nit:
>
>            // Check type since it is destroyed when the type is deleted
>            if(GetStyleSheet() && HAS_BASE(SfxStyleSheet, mpStyleSheet))
>
>        If indeed we don't need to check the type, we should prolly drop the
> comment as well [ did you look at the commit that added those comments
> in the history ? with git annotate - perhaps there is something
> interesting there ].
No i didnt look at the code history, i saw alot of stuff that could
possibly can get improved but didnt want to change lots of code, you know
how fragile is the codebase. The most usage of the RTTI is for ui, views
and shell classes.


>        Anyhow - great to have you cleaning this up.
>
>        ATB,
>
>                Michael.
>
> --
> michael.meeks@suse.com  <><, Pseudo Engineer, itinerant idiot