590179 - Enable -fomit-frame-pointer for the full build on OS X

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Robert Sayre]

bug 492688 turned this on for the JS engine.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is a noticeable speedup on some dromaeo tests (we're talking things like 10% improvement).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Pretty straightforward change; the only weirdness is the xptcall stuff, which the comments explain.

If we don't want to do this, I'll probably start adding to some MODULE_OPTIMIZE_FLAGS, since this is a noticeable win in some code...

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #2)
> If we don't want to do this, I'll probably start adding to some
> MODULE_OPTIMIZE_FLAGS, since this is a noticeable win in some code...

Doing this across the board (even with a little ugliness in xptcall) is far better than sprinkling ifdefed MODULE_OPTIMIZE_FLAGS across the codebase.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 469763 [details] [diff] [review]
Proposed patch

I can't see how an extra register would be anything but a win everywhere.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 469763 [details] [diff] [review]
Proposed patch

a=beltzner

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Not sure I'd block on this if it turned out to cause regressions, but definitely approved for landing!

Comment 7

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/a6c18a123fbb

Comment 8

[[e-mail]]

Comment on attachment 469763 [details] [diff] [review]
Proposed patch

>+# If we compile xptcinvoke_unixish_x86.cpp with -fomit-frame-pointer
>+# we end up crashing on startup, presumably because of the %esp munging we do when KEEP_STACK_16_BYTE_ALIGNED.  So let's not use that flag here.
>+MODULE_OPTIMIZE_FLAGS=-03

"its -O3 the letter, not -03 the number". (http://funroll-loops.info/)

I'm surprised this didn't warn... (or asplode.)

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It probably did warn, but warnings are hard to see.

Comment 10

[(no longer active)]

And yes, it turned all of Mac runs orange.

Comment 11

[(no longer active)]

Backout:

http://hg.mozilla.org/mozilla-central/rev/69c903db2efd

Comment 12

[Boris Zbarsky [:bzbarsky]]

> It probably did warn, but warnings are hard to see.

Without the patch:

mozilla% make -C ../obj-firefox-opt/xpcom/reflect/xptcall/src/md/unix/ | & grep warning | sed 's/.*:[0-9]*://' | sort -u
 warning: ‘saved_esp’ is used uninitialized in this function

With the patch:

mozilla% make -C ../obj-firefox-opt/xpcom/reflect/xptcall/src/md/unix/ | & grep warning | sed 's/.*:[0-9]*://' | sort -u
 warning: ‘saved_esp’ is used uninitialized in this function

So no, it did not warn.

My apologies for the orange; I did also kick off a try server build last night, meaning to check it before pushing today, but it didn't have results before I had to go to bed.  It's obviously orange too.  I should have mentioned that in the bug; I wasn't expecting someone else to push the patch for me.  :(

Looking into the failures.

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK, looks like I failed to actually test my final patch, after moving that MODULE_OPTIMIZE_FLAGS into the ifdef.  Seems like MODULE_OPTIMIZE_FLAGS needs to be before the config.mk include to work here?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Yeah, indeed, since config.mk is what looks at that variable.  Made a change to fix that locally and pushed to try.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 16

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/fa9a6337669b

Comment 17

[Boris Zbarsky [:bzbarsky]]

Talos data:
  Improvement: Dromaeo (CSS) increase 3.67% on MacOSX 10.6.2 Firefox
  Improvement: Dromaeo (DOM) increase 5.64% on MacOSX 10.6.2 Firefox
  Improvement: Txul decrease 2.94% on MacOSX 10.6.2 Firefox