967300 - Use atomics with gcc in cairo

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Attachment: Use atomics with gcc in cairo

This was an oversight.

Comment 1

[Michael Shal [:mshal]]

Comment on attachment 8369726 [details] [diff] [review]
Use atomics with gcc in cairo

jmuizelaar and I chatted on IRC a few days ago - this patch fails to compile due to the fact that the HAVE_INTEL_ATOMIC_PRIMITIVES code patch relies on SIZEOF_VOID_P, SIZEOF_INT, etc, which normally would come from cairo's configure script.

One way to fix this would be to add the AC_CHECK_SIZEOF(void *) checks to the top-level configure.in, and then the defines would be picked up by cairo from mozilla-config.h. glandium: Is that an acceptible place for those? Or is there a better way to define them locally under the gfx/cairo tree?

Comment 2

[Mike Hommey [:glandium]]

There's probably nothing better to do at the moment.

Comment 3

[Ginn Chen]

FWIW:
If we have
#define HAVE_STDINT_H 1
#define HAVE_UINT64_T 1
in cairo/src/cairoint.h and cairo/cairo/src/cairo.h, we will save the call of cairo_int32x32_64_mul(), etc.

It will improve performance a little bit.

Probably should file another bug, but maybe we will fix it together.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: enable cairo's atomic support on gcc-esque compilers

This is just Jeff's original patch + the modifications suggested by mshal and
reluctantly affirmed by glandium.  A local build seems to confirm everything works.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> Created attachment 8549862 [details] [diff] [review]
> enable cairo's atomic support on gcc-esque compilers
> 
> This is just Jeff's original patch + the modifications suggested by mshal and
> reluctantly affirmed by glandium.  A local build seems to confirm everything
> works.

...well, it would have if tree-wide builds worked correctly.  Looks like some media/mtransport/ goop magically triggers things off our shiny new AC_CHECK_SIZEOF results.  Sigh.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: enable cairo's atomic support on gcc-esque compilers

Here's a better version of the patch that doesn't require running programs at
configure time to determine the sizes of fundamental datatypes.

Comment 7

[Nathan Froyd [:froydnj]]

Seems sort of busted that we'll have this enabled on Linux and Mac, but not Windows...or am I ignorant about how our gfx setup works?  Can we import a patch from upstream or write basic Windows-only atomic ops support?

Comment 9

[Jeff Muizelaar [:jrmuizel]]

I'll write and upstream a windows implementation.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/70aa258394bf

Comment 11

[Jeff Muizelaar [:jrmuizel]]

This has been backed out.
https://hg.mozilla.org/mozilla-central/rev/dc194ba77d7f

Comment 12

[Nathan Froyd [:froydnj]]

Jeff, have you been able to look at why this causes performance regressions?

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Unfortunately, not yet. Let me do some sample try pushes to try to narrow things down.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Before:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcc5f4aa1198

After:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a394c5ce3a4c

Everything but image-surface disabled:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e708c51709c

Everything disabled:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=904cdcf39cbe

image-surface disabled:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7510cf422a89

Comment 15

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> Everything disabled:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=904cdcf39cbe

https://treeherder.mozilla.org/#/jobs?repo=try&revision=51d7da1259ca

Comment 16

[Jeff Muizelaar [:jrmuizel]]

It looks like the performance regression wasn't actually a regression.

I've relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/604003669a62

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/604003669a62