Merge lp://qastaging/~smspillaz/compiz-core/compiz-core.optimization-inlining into lp://qastaging/compiz-core

Not really relevant to the change, but as "Region handle () const" compiles there's already a declaration of Region visible - so why is priv a "void*" and cast all the while?

priv is a void* because in the penultimate revision before that change was merged, priv could represent different types (Region or PrivateRegion). Now that PrivateRegion is gone however you are correct that priv could be simply "Region priv".

I haven't tested it yet, but I'm pretty sure this branch should contain a CORE_ABIVERSION bump.

> I haven't tested it yet, but I'm pretty sure this branch should contain a
> CORE_ABIVERSION bump.

Good catch

8 - Region handle () const;
9 + Region handle () const { return static_cast<Region> (priv); }

I noticed also that CompRegion::handle is the most commonly called function in compiz. However it is also tiny and only accounts for 1% of compiz CPU usage. Which is why I didn't touch it.

Another problem with changing handle() is that if we change the implementation of CompRegion in future then we will need to change handle() again and that would be another ABI bump.

I think a better solution to solve all of these problems is to keep handle() unchanged, and add a new method:
    private:
        Region privHandle () const { return static_cast<Region> (priv); }
for use in region.cpp only.

Or simply:
    #define REGIONHANDLE (static_cast<Region> (priv))
in region.cpp

Also, testing this code I notice another windows-shifted-horizontally regression (though I suspect it's the same one and we haven't fixed the root cause yet). When a window is maximized or restored, it's contents are shifted to the right/left of where they should be. When you move the window, the shift corrects itself.

:(

Though it's possible the shift regression would be fixed by merging the other branch Sam mentions...

Confirmed: The shift regression seen with this branch is fixed if you also merge:
lp:~smspillaz/compiz-core/compiz-core.work_923683
But that branch has its own problems.

> 8 - Region handle () const;
> 9 + Region handle () const { return static_cast<Region> (priv); }
>
> I noticed also that CompRegion::handle is the most commonly called function in
> compiz. However it is also tiny and only accounts for 1% of compiz CPU usage.
> Which is why I didn't touch it.
>
> Another problem with changing handle() is that if we change the implementation
> of CompRegion in future then we will need to change handle() again and that
> would be another ABI bump.

We change the ABI for other reasons too, and I really don't think the implementation of CompRegion is going to change (except, perhaps to cairo_region_t), so this is just a total non-issue.

>
> I think a better solution to solve all of these problems is to keep handle()
> unchanged, and add a new method:
> private:
> Region privHandle () const { return static_cast<Region> (priv); }
> for use in region.cpp only.

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.

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.

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.

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.

If I'm wrong, then please log a bug with the profile data showing why all this existing code needs optimizing.

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.
>

Still causes the shift regression, despite us thinking it was fixed by:
https://code.launchpad.net/+branch/~smspillaz/compiz-core/compiz-core.backport_2988_work_923683
It seems that fix has not worked.

Aside from the shift regression which still needs fixing, I strongly recommend not breaking the ABI (now and again in future) by inlining handle() or border(). If anything, I suggest adding new inline functions "inlineHandle" and "inlineBorder". Ideally they should be private. If not private, then they should be documented as "only safe to use in core or core plugins".

CORE_ABIVERSION still needs bumping too.

And yes, when I say "causes the shift regression" I actually mean "triggers the shift regression". I'm pretty sure the bug is not in the below diff, only exploited by it.