Closed Bug 806819 Opened 12 years ago Closed 10 years ago

Define FORCE_PR_LOG globally so release builds have all NSPR logging available

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: ted, Assigned: erahm)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

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...
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.
Depends on: 1072605
Depends on: 1073712
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
Depends on: 1074415
Also note we'll be able to add files back to unified building that were excluded due to using FORCE_PR_LOGGING.
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.
OS: Windows 7 → All
Hardware: x86_64 → All
Depends on: 1075966
Depends on: 1075984
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Attachment #8498446 - Flags: review?(gps)
Attachment #8498447 - Flags: review?(ted)
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+
Attachment #8498451 - Flags: review?(rjesup)
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)
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)
Depends on: 1076417
Attachment #8498448 - Flags: review?(ehsan.akhgari) → review+
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+
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+
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)
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.
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+
(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
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)
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)
trying all builds, all tests: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a7a4401ceea5

XP seems happy now.
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.