Code review comment for lp://qastaging/~smspillaz/compiz-core/compiz-core.optimization-inlining

On Mon, 5 Mar 2012, Daniel van Vugt wrote:

> Review: Disapprove
>
> CompRegion::handle()
> I think this may be worth inlining privately. As mentioned earlier. We shouldn't make handle() inline for external users of the class. FYI, CompRegion::handle() is consistently the single most called function in compiz, however it never accounts for more than 1.3% of the total CPU usage.
>

Ack, although other plugins use the handle directly.

> compiz::window::Geometry::border ()
> Makes no sense to inline AFAICS. It may well be around the 25th most called function, however it only accounts for 0.07% of the CPU usage.
>

Why not ? Avoids the JMP

> CompWindow::borderRect()
> Does not need to optimizing at all. Is almost never called and contributes absolutely nothing to the total CPU usage.
>

> *Rect()
> Contribute almost nothing measurable to the CPU usage.
>

They account for about 5% ish of usage, and are called during paint
functions. There is no reason for them to operate in the way that they
currently do. The constructors also involve XCreateRegion and
XDestroyRegion pairs.

Just because the optimization isn't "significant" that doesn't mean that
it is useless. In fact, the code as it was written makes no sense
whatsoever, and there isn't any reason to keep it as it is.

> All-in-all, these optimizations don't seem to provide any significant benefit. I think CompRegion::handle() could be optimized using a private inline version of that function. But even then, the maximum gain is going to be about 1%. The rest of the changes break the ABI, cause a regression, and do not appear to be required at all.
>

The regression is unrelated, see
lp:~smspillaz/compiz-core/compiz-core.backport_2988_work_923683

> If I'm wrong, then please log a bug with the profile data showing why all this existing code needs optimizing.
> --
> https://code.launchpad.net/~smspillaz/compiz-core/compiz-core.optimization-inlining/+merge/95094
> You are the owner of lp:~smspillaz/compiz-core/compiz-core.optimization-inlining.
>