Define FORCE_PR_LOG globally so release builds have all NSPR logging available

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: ted, Assigned: erahm)

Tracking

(Blocks 1 bug)

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

We have a lot of modules that #define FORCE_PR_LOG so that their NSPR logging works in release builds. NSPR logging can be really useful, so I think if it's not a perf hit we should just #define this globally.
In many of the modules which don't thus define it it _would_ be a perf hit.  We need to go through and audit them first...

Comment 2

7 years ago
Defining globally would also fix the silent bustage that can happen if you accidentally include a header that includes any IPDL stuff before you #define FORCE_PR_LOG.  See bug 545995.
Assignee

Updated

5 years ago
Depends on: 1072605
Assignee

Updated

5 years ago
Depends on: 1073712
Assignee

Comment 3

5 years ago
Some measurements were done in bug 881389 comment 63, bug 881389 comment 72, bug 881389 comment 75 to measure the impact of enabling nspr logging in non-debug builds. The gist is there's minimal overhead of enabling logging (and leaving the default output level to None) for non-debug builds in both performance and size.

My recommendation is to define FORCE_PR_LOGGING if |--disable-logging| is not specified at configure time [1]. Then we'd want to remove all instances where we currently define it by hand in mozilla code. Additionally we'll need to deal with some build bustage from this (I already have a few bugs filed) where we've made some bad assumptions that PR_LOGGING == DEBUG.

The final question is whether or not we want this enabled in proper releases or just in nightly/aurora(/beta). If I had my way we'd enable it all the way through, but if we want to be conservative just having it on nightly/aurora would still be a big win.

[1] http://hg.mozilla.org/mozilla-central/annotate/5e704397529b/configure.in#l6851
Assignee

Updated

5 years ago
Depends on: 1074415
Assignee

Comment 4

5 years ago
Also note we'll be able to add files back to unified building that were excluded due to using FORCE_PR_LOGGING.
Assignee

Updated

5 years ago
Depends on: 1075093
I would vote for enabling it in release as well to minimize differences between branches. If we're comfortable enabling it for Nightly/Aurora then it should be fine for Beta/Release.
Assignee

Updated

5 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
Assignee

Updated

5 years ago
Depends on: 1075966
Assignee

Updated

5 years ago
Depends on: 1075984
Assignee

Updated

5 years ago
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Assignee

Updated

5 years ago
Attachment #8498446 - Flags: review?(gps)
Assignee

Updated

5 years ago
Attachment #8498447 - Flags: review?(ted)
Assignee

Updated

5 years ago
Attachment #8498451 - Flags: review?(gps)
Attachment #8498446 - Flags: review?(gps) → review+
Comment on attachment 8498451 [details] [diff] [review]
Part 5: Update webrtc moz.build

Review of attachment 8498451 [details] [diff] [review]:
-----------------------------------------------------------------

You don't need a build peer review on this. But I'll rubber stamp it.

FWIW, the previous patch is also a rubber stamp. You should really ask whoever is reviewing the C++ to give it the OK from a policy front.
Attachment #8498451 - Flags: review?(gps) → review+
Assignee

Updated

5 years ago
Attachment #8498451 - Flags: review?(rjesup)
Assignee

Comment 12

5 years ago
Comment on attachment 8498448 [details] [diff] [review]
Part 3: Remove redundant FORCE_PR_LOG entries

Review of attachment 8498448 [details] [diff] [review]:
-----------------------------------------------------------------

Ehsan, I'm tagging you because you seem to have cared about this stuff in the past. Feel free to recommend others.
Attachment #8498448 - Flags: review?(ehsan.akhgari)
Assignee

Comment 13

5 years ago
Comment on attachment 8498449 [details] [diff] [review]
Part 4: Add files that were excluded from unified builds back in

Review of attachment 8498449 [details] [diff] [review]:
-----------------------------------------------------------------

Ehsan, I'm tagging you because you seem to have cared about this stuff in the past. Feel free to recommend others. Adding glandium for build perspective.
Attachment #8498449 - Flags: review?(mh+mozilla)
Attachment #8498449 - Flags: review?(ehsan.akhgari)
Assignee

Updated

5 years ago
Depends on: 1076417

Updated

5 years ago
Attachment #8498448 - Flags: review?(ehsan.akhgari) → review+

Comment 14

5 years ago
Comment on attachment 8498449 [details] [diff] [review]
Part 4: Add files that were excluded from unified builds back in

Review of attachment 8498449 [details] [diff] [review]:
-----------------------------------------------------------------

This is *wonderful*!

Please make sure that this builds on all platforms both debug and opt on the try server.  I've seen a ton of cases where this kind of things fails to build only one platform and/or one configuration.

::: gfx/thebes/moz.build
@@ +205,5 @@
>      # on X11, gfxDrawable.cpp includes X headers for an old workaround which
>      # we could consider removing soon (affects Ubuntus older than 10.04 LTS)
>      # which currently prevent it from joining UNIFIED_SOURCES.
>      'gfxDrawable.cpp',
> +    # Includes mac system header conflicting with point/size

Nit: I think this comment only applies to gfxPlatform.cpp, so please include that name here.

::: media/mtransport/build/moz.build
@@ +26,5 @@
>  ]
>  
>  include('../objs.mozbuild')
>  
> +# These files cannot be built in unified mode because mtransport does bad things

It would be nice if you could be more specific (it's fine as it is too, but this comment doesn't deliver much information.)

::: media/mtransport/standalone/moz.build
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  include('../objs.mozbuild')
>  
> +# These files cannot be built in unified mode because mtransport does bad things.

Ditto.
Attachment #8498449 - Flags: review?(ehsan.akhgari) → review+

Updated

5 years ago
Blocks: unified
Comment on attachment 8498451 [details] [diff] [review]
Part 5: Update webrtc moz.build

Review of attachment 8498451 [details] [diff] [review]:
-----------------------------------------------------------------

r+ under the assumption that all other issues dealt with....  I'm not r+ing the change in FORCE_PR_LOG policy here, just that if that is changed this is reasonable.
Attachment #8498451 - Flags: review?(rjesup) → review+
Assignee

Comment 16

5 years ago
To help get better insight into any possible perf regressions we're running these changes against the talos test suite [1]. jmaher is helping me interpret the results and has confirmed the tests run on real hardware [2]. The initial results on linux look pretty good.

[1] https://tbpl.mozilla.org/?tree=Try&rev=5dc90dc8178a
[2] https://wiki.mozilla.org/Buildbot/Talos/Misc#Hardware_Profile_of_machines_used_in_automation
Comment on attachment 8498449 [details] [diff] [review]
Part 4: Add files that were excluded from unified builds back in

Review of attachment 8498449 [details] [diff] [review]:
-----------------------------------------------------------------

Ehsan's review is enough.
Attachment #8498449 - Flags: review?(mh+mozilla)
Assignee

Comment 18

5 years ago
Rebased, and tweaked the unified build split. try looks happy: https://tbpl.mozilla.org/?tree=Try&rev=fd1a477276c5

Just waiting on Ted's input and the talos results.
all is done on the try run except for 10.8, and the results are great, no improvements and no regressions.

Comment 20

5 years ago
Eric, can you please make sure to announce this on dev.platform when you're ready to land?  I think many people will find this interesting.  Thanks!
Attachment #8498447 - Flags: review?(ted) → review+
Assignee

Comment 21

5 years ago
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #14)
> Comment on attachment 8498449 [details] [diff] [review]
> Part 4: Add files that were excluded from unified builds back in
> 
> Review of attachment 8498449 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is *wonderful*!
> 
> Please make sure that this builds on all platforms both debug and opt on the
> try server.  I've seen a ton of cases where this kind of things fails to
> build only one platform and/or one configuration.

Yes, I've definitely been iterating on the moz.build changes :)

> ::: gfx/thebes/moz.build
> @@ +205,5 @@
> >      # on X11, gfxDrawable.cpp includes X headers for an old workaround which
> >      # we could consider removing soon (affects Ubuntus older than 10.04 LTS)
> >      # which currently prevent it from joining UNIFIED_SOURCES.
> >      'gfxDrawable.cpp',
> > +    # Includes mac system header conflicting with point/size
> 
> Nit: I think this comment only applies to gfxPlatform.cpp, so please include
> that name here.

Updated locally.

> ::: media/mtransport/build/moz.build
> @@ +26,5 @@
> >  ]
> >  
> >  include('../objs.mozbuild')
> >  
> > +# These files cannot be built in unified mode because mtransport does bad things
> 
> It would be nice if you could be more specific (it's fine as it is too, but
> this comment doesn't deliver much information.)

Yeah, I was intentionally vague. I don't think the previous comment was necessarily correct, all sorts of stuff breaks when switching to unified. I'll try again and see if I can be more specific.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #20)
> Eric, can you please make sure to announce this on dev.platform when you're
> ready to land?  I think many people will find this interesting.  Thanks!

Sent out a message, waiting for any last minute feedback:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/TvU4_wFXCdY
Assignee

Comment 22

5 years ago
try is looking good: https://tbpl.mozilla.org/?tree=Try&rev=cf7bf6268a9b

jmaher took a look at the 10.8 talos result and gave it the thumbs up. At this point b/w local testing, Talos, mochitests, etc I feel pretty good about landing this.
Backed these out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b92b206a2760 for WinXP test failures: https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=009ae35b0c67&searchQuery=Windows%20XP

The tests are passing on Windows 7, so I guess it's something XP-specific? I did trigger a WinXP mochitest-1 run on your try push from comment 22, so we'll see if it shows up there.
Flags: needinfo?(erahm)
Assignee

Comment 25

5 years ago
nthomas identified this as a startup crash, "firefox.exe Entry Point Not Found: The procedure entry point K32GetProcessImageFileNameW could not be found in the dynamic link library KERNEL32.dll"

Which makes sense b/c GetProcessImageFileNameW is in PSAPI.dll on XP [1], so now to figure out why we're attempting to load this from kernel32.dll. A DXR search [2] shows only a few uses, so it should be somewhat easy to track down.

[1] http://msdn.microsoft.com/en-us/library/windows/desktop/ms683217%28v=vs.85%29.aspx
[2] http://dxr.mozilla.org/mozilla-central/search?q=GetProcessImageFileName&case=true
Flags: needinfo?(erahm)
Assignee

Comment 26

5 years ago
trying all builds, all tests: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a7a4401ceea5

XP seems happy now.
Assignee

Comment 27

5 years ago
dt1 kept failing on linux debug, but they've disabled the offending test in bug 1076830, so here we go again:  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ffd77be93a56
Thanks for doing the legwork to fix this!
Depends on: 1081131
You need to log in before you can comment on or make changes to this bug.