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)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: ted, Assigned: erahm)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
(deleted),
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gps
:
review+
jesup
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•12 years ago
|
||
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•12 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 | ||
Comment 3•10 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 | ||
Comment 4•10 years ago
|
||
Also note we'll be able to add files back to unified building that were excluded due to using FORCE_PR_LOGGING.
Reporter | ||
Comment 5•10 years ago
|
||
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•10 years ago
|
OS: Windows 7 → All
Hardware: x86_64 → All
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•10 years ago
|
||
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8498446 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8498447 -
Flags: review?(ted)
Assignee | ||
Updated•10 years ago
|
Attachment #8498451 -
Flags: review?(gps)
Updated•10 years ago
|
Attachment #8498446 -
Flags: review?(gps) → review+
Comment 11•10 years ago
|
||
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•10 years ago
|
Attachment #8498451 -
Flags: review?(rjesup)
Assignee | ||
Comment 12•10 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•10 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)
Updated•10 years ago
|
Attachment #8498448 -
Flags: review?(ehsan.akhgari) → review+
Comment 14•10 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+
Comment 15•10 years ago
|
||
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•10 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 17•10 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's review is enough.
Attachment #8498449 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 18•10 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.
Comment 19•10 years ago
|
||
all is done on the try run except for 10.8, and the results are great, no improvements and no regressions.
Comment 20•10 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!
Reporter | ||
Updated•10 years ago
|
Attachment #8498447 -
Flags: review?(ted) → review+
Assignee | ||
Comment 21•10 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•10 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.
Assignee | ||
Comment 23•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/70a167982c3f
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e25a2a8d4af4
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f06cd735b5b3
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5a57f87f5061
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/009ae35b0c67
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•10 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•10 years ago
|
||
trying all builds, all tests: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a7a4401ceea5
XP seems happy now.
Assignee | ||
Comment 27•10 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
Assignee | ||
Comment 28•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d2c37412b5ee
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0bed6fc0b0b7
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f15a0fd7e3db
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9d6589539f8a
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7b16babf6a73
https://hg.mozilla.org/mozilla-central/rev/d2c37412b5ee
https://hg.mozilla.org/mozilla-central/rev/0bed6fc0b0b7
https://hg.mozilla.org/mozilla-central/rev/f15a0fd7e3db
https://hg.mozilla.org/mozilla-central/rev/9d6589539f8a
https://hg.mozilla.org/mozilla-central/rev/7b16babf6a73
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Reporter | ||
Comment 30•10 years ago
|
||
Thanks for doing the legwork to fix this!
You need to log in
before you can comment on or make changes to this bug.
Description
•