311980 - Speed up _cairo_fixed_from_double

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[tor]

Cairo's current implementation of gradients makes heavy use of
_cairo_fixed_from_double, which vc6's compiler can't seem to do a good job of
optimizing.  Using a neat trick to speed it up improved our gradient performance
tests by about 25% on windows.

Comment 1

[tor]

Attachment: faster _cairo_fixed_from_double()

Comment 2

[Carl Worth]

The patch looks pretty good to me. I haven't tested it much yet, but the
approach seems fine, (I'm glad to see a link to an explanation in the patch).
And obviously, the fix has a nice performance impact as you have measured.

The patch does make assumptions about the floating-point representation, so
before I commit this to cairo proper, I'd like to see this version made
conditional on a configure-time check so we have better assurance that it will
actually work on a wide range of platforms.

Comment 3

[tor]

Drivers aren't going to allow this on the branch, and there are plans to change
how cairo implements gradients going forward.  Closing as WONTFIX.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Won't _cairo_fixed_from_double get used a good bit any time we call into cairo from layout, esp if we change to float for nscoord?  Might be worth it to check this in anyway; cairo can use all the perf help it can get....

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, this is quite good; I'm seeing fixed_from_double a lot in the profiles.  It should also be inline; right now it's being called as a function everywhere.

Comment 6

[tor]

Turning it into a macro would involve a sweep through the cairo tree modifying the  calls to _cairo_fixed_from_double in argument lists, since we need to create a block for the temporary variable used to shift types.

Comment 7

[Stuart Parmenter]

(In reply to comment #4)
> Won't _cairo_fixed_from_double get used a good bit any time we call into cairo
> from layout, esp if we change to float for nscoord?
> 

no.  this is an internal cairo function.  all cairo APIs take doubles so anything from layout gets converted in to a double and then to fixed for some things inside cairo.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #6)
> Turning it into a macro would involve a sweep through the cairo tree modifying
> the  calls to _cairo_fixed_from_double in argument lists, since we need to
> create a block for the temporary variable used to shift types.

My plan was to just define all the cairo-fixed functions as inline in the header file; "static inline" isn't valid, but I think that's effectively the semantics we'd get.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> anything from layout gets converted in to a double and then to fixed for some
> things inside cairo.

Right.  And the latter conversion should be fast if we can make it so.

Comment 10

[Jonathan Watt [:jwatt]]

Daniel Amelang got some code checked into cairo for this.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yep, this has been fixed.