Closed
Bug 1257434
Opened 8 years ago
Closed 8 years ago
Move some profiling-related options to moz.configure
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
(Blocks 2 open bugs)
Details
Attachments
(7 files, 1 obsolete file)
58 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
ted
:
review+
|
Details |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40739/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40739/
Attachment #8731600 -
Flags: review?(ted)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40741/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40741/
Attachment #8731601 -
Flags: review?(ted)
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40743/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40743/
Attachment #8731602 -
Flags: review?(ted)
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40745/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40745/
Attachment #8731603 -
Flags: review?(ted)
Assignee | ||
Comment 5•8 years ago
|
||
Remove the unused DEFINES set in js/xpconnect/shell/moz.build, subsequently making the MOZ_CALLGRIND subst unused, so don't add a set_config for it. Review commit: https://reviewboard.mozilla.org/r/40747/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40747/
Attachment #8731605 -
Flags: review?(ted)
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40749/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40749/
Attachment #8731606 -
Flags: review?(ted)
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40751/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40751/
Attachment #8731607 -
Flags: review?(ted)
Assignee | ||
Comment 8•8 years ago
|
||
While not directly related to the others from this bug, I stumbled upon this while looking why there were references to MOZ_DMD in js/src/old-configure.in while it was never set there. MOZ_DEMANGLE_SYMBOLS is tied to MOZ_STACKWALKING, which is not set in js/src/old-configure.in. MOZ_COMPONENTS_VERSION_SCRIPT_LDFLAGS is specific to building XPCOM binary components, which there are none of under js/src. Review commit: https://reviewboard.mozilla.org/r/40753/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40753/
Attachment #8731608 -
Flags: review?(ted)
Updated•8 years ago
|
Attachment #8731600 -
Flags: review?(ted) → review+
Comment 9•8 years ago
|
||
Comment on attachment 8731600 [details] MozReview Request: Bug 1257434 - Move --enable-systrace to moz.configure. r=ted https://reviewboard.mozilla.org/r/40739/#review37239 ::: toolkit/moz.configure:11 (Diff revision 1) > > + > +# Profiling > +# ============================================================== > +# Some of the options here imply an option from js/moz.configure, > +# so, need to be declared before the include. Will we error reading moz.configure if someone screws this up, or only if the option gets used?
Comment 10•8 years ago
|
||
Comment on attachment 8731601 [details] MozReview Request: Bug 1257434 - Move --enable-jprof to moz.configure. r=ted https://reviewboard.mozilla.org/r/40741/#review37241
Attachment #8731601 -
Flags: review?(ted) → review+
Comment 11•8 years ago
|
||
Comment on attachment 8731602 [details] MozReview Request: Bug 1257434 - Move MOZ_ENABLE_PROFILER_SPS to moz.configure. r=ted https://reviewboard.mozilla.org/r/40743/#review37243 ::: toolkit/moz.configure:39 (Diff revision 1) > > +@depends(target) > +def sps_profiler(target): > + if target.os == 'Android': > + return target.cpu in ('arm', 'x86') > + elif target.kernel == 'Linux': The original code is checking `OS_TARGET`, is there a reason you're using `target.kernel` here, other than that you have it now? Specifically, it seems like the case for Darwin would be better served just checking `target.os == 'OSX'`. Also, looking at the original, it struck me that you could also write this like: ``` return (target.os, target.cpu) in ( ('Android', 'arm'), ('Android', 'x86'), ... ) ```
Attachment #8731602 -
Flags: review?(ted) → review+
Comment 12•8 years ago
|
||
Comment on attachment 8731603 [details] MozReview Request: Bug 1257434 - Move --enable-instruments to moz.configure. r=ted https://reviewboard.mozilla.org/r/40745/#review37245 ::: js/moz.configure:81 (Diff revision 1) > + help='Enable instruments remote profiling') > + > +@depends('--enable-instruments', target) > +def instruments(value, target): > + if value and target.os != 'OSX': > + error('--enable-instruments cannot be used when targetting %s' I can't believe we didn't have this check in old-configure! (Also spelling nit: 'targeting'.)
Attachment #8731603 -
Flags: review?(ted) → review+
Updated•8 years ago
|
Attachment #8731605 -
Flags: review?(ted) → review+
Comment 13•8 years ago
|
||
Comment on attachment 8731605 [details] MozReview Request: Bug 1257434 - Move --enable-callgrind to moz.configure. r=ted https://reviewboard.mozilla.org/r/40747/#review37247 ::: js/moz.configure:95 (Diff revision 1) > + help='Enable callgrind profiling') > + > +@depends('--enable-callgrind') > +def callgrind(value): > + if value: > + set_define('MOZ_CALLGRIND', '1') I know we discussed the proliferation of defines and the rebuild-the-world issues they cause a little bit at the work week. In light of that, do you think this is the right direction to go with this value? It's only used in three source files, AFAICT: https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Profilers.cpp https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Profilers.h https://dxr.mozilla.org/mozilla-central/source/js/src/shell/js.cpp and the Profilers.h header is only included by Profilers.cpp, so this could instead be a subst + set in DEFINES in js/src/moz.build + js/src/shell/moz.build.
Comment 14•8 years ago
|
||
Comment on attachment 8731606 [details] MozReview Request: Bug 1257434 - Move --enable-dmd to moz.configure. r=ted https://reviewboard.mozilla.org/r/40749/#review37249 ::: old-configure.in (Diff revision 1) > dnl ======================================================== > dnl = Enable DMD > dnl ======================================================== > > -MOZ_ARG_ENABLE_BOOL(dmd, > -[ --enable-dmd Enable DMD; also enables jemalloc, replace-malloc and profiling], The bits in the help about "also enables..." got lost in translation. Does that matter?
Attachment #8731606 -
Flags: review?(ted) → review+
Updated•8 years ago
|
Attachment #8731607 -
Flags: review?(ted) → review+
Comment 15•8 years ago
|
||
Comment on attachment 8731607 [details] MozReview Request: Bug 1257434 - Move --enable-vtune and --enable-profiling to moz.configure. r=ted https://reviewboard.mozilla.org/r/40751/#review37251 ::: js/moz.configure:113 (Diff revision 1) > + enabled = bool(value) > + if not enabled and profiling: > + # --enable-profiling implies --enable-vtune on Windows and non-Android > + # Linux. > + enabled = target.kernel == 'WINNT' or (target.kernel == 'Linux' and > + target.os == 'GNU') I'm not really wild about `target.os == 'GNU'` signifying Desktop Linux. You've been in Debian land too long! I think this is going to confuse a lot of people. ::: js/moz.configure:128 (Diff revision 1) > +def profiling(value, vtune): > + enabled = bool(value) > + if not enabled and vtune: > + # If vtune was explicitly enabled and profiling explicitly disabled, > + # this is an error (same error message as the one with conflicts from > + # imply_option, which we can't use because of the mutual dependency. The difference in how this is currently implemented in old-configure vs. js/src/old-configure is painful. They do things in opposite ways! Is there a more sensible way we could implement this? ::: js/moz.configure:139 (Diff revision 1) > + set_config('MOZ_PROFILING', '1') > + set_define('MOZ_PROFILING', '1') > + add_old_configure_assignment('MOZ_PROFILING', '1') > + # Because of how we pass down options to js/src/configure, we need > + # to explicitly send --enable-profiling to old-configure. > + add_old_configure_arg('--enable-profiling') At what point do we get to remove this?
Comment 16•8 years ago
|
||
Comment on attachment 8731608 [details] MozReview Request: Bug 1257434 - Remove MOZ_DEMANGLE_SYMBOLS and MOZ_COMPONENTS_VERSION_SCRIPT_LDFLAGS from js/src/old-configure.in https://reviewboard.mozilla.org/r/40753/#review37253 Whee, cargo-culting!
Attachment #8731608 -
Flags: review?(ted) → review+
Assignee | ||
Comment 17•8 years ago
|
||
https://reviewboard.mozilla.org/r/40743/#review37243 > The original code is checking `OS_TARGET`, is there a reason you're using `target.kernel` here, other than that you have it now? Specifically, it seems like the case for Darwin would be better served just checking `target.os == 'OSX'`. > > Also, looking at the original, it struck me that you could also write this like: > ``` > return (target.os, target.cpu) in ( > ('Android', 'arm'), > ('Android', 'x86'), > ... > ) > ``` Note OS_TARGET is not exactly target.os. Yes, this can be confusing, but if we want to sort out the mess from OS_TARGET, OS_TEST, etc. we have to make things confusing, otherwise, we stay with the mess forever. But sure, the OSX case doesn't actually need to check for Darwin. The `(target.os, target.cpu) in` thing you're suggesting would not work because OS_TARGET is not exactly target.os.
Assignee | ||
Comment 18•8 years ago
|
||
https://reviewboard.mozilla.org/r/40739/#review37239 > Will we error reading moz.configure if someone screws this up, or only if the option gets used? Unfortunately the latter. But I'm thinking of ways we could change that.
Assignee | ||
Comment 19•8 years ago
|
||
https://reviewboard.mozilla.org/r/40747/#review37247 > I know we discussed the proliferation of defines and the rebuild-the-world issues they cause a little bit at the work week. In light of that, do you think this is the right direction to go with this value? It's only used in three source files, AFAICT: > https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Profilers.cpp > https://dxr.mozilla.org/mozilla-central/source/js/src/builtin/Profilers.h > https://dxr.mozilla.org/mozilla-central/source/js/src/shell/js.cpp > > and the Profilers.h header is only included by Profilers.cpp, so this could instead be a subst + set in DEFINES in js/src/moz.build + js/src/shell/moz.build. I'll file a followup.
Assignee | ||
Comment 20•8 years ago
|
||
https://reviewboard.mozilla.org/r/40749/#review37249 > The bits in the help about "also enables..." got lost in translation. Does that matter? That's part of why I want to change how implied options work, so that they can be documented in most cases, and can be known beforehand.
Assignee | ||
Comment 21•8 years ago
|
||
https://reviewboard.mozilla.org/r/40751/#review37251 > I'm not really wild about `target.os == 'GNU'` signifying Desktop Linux. You've been in Debian land too long! I think this is going to confuse a lot of people. What do you suggest? The interesting property of target.os == 'GNU' is that is applies to GNU/Linux, GNU/kFreeBSD and GNU/Hurd (essentially anything based on glibc). In fact, there are many of the things we limit to Linux that would likely work on all of them, possibly even SPS. Now, I do agree that it makes the non-Android Linux-only case awkward. > The difference in how this is currently implemented in old-configure vs. js/src/old-configure is painful. They do things in opposite ways! > > Is there a more sensible way we could implement this? We can certainly invert it. Does it matter? > At what point do we get to remove this? When js/src/configure dies.
Comment 22•8 years ago
|
||
https://reviewboard.mozilla.org/r/40751/#review37251 > What do you suggest? The interesting property of target.os == 'GNU' is that is applies to GNU/Linux, GNU/kFreeBSD and GNU/Hurd (essentially anything based on glibc). In fact, there are many of the things we limit to Linux that would likely work on all of them, possibly even SPS. Now, I do agree that it makes the non-Android Linux-only case awkward. If your intention is to have it signify the libc in use, then maybe it should be explicitly `target.libc`? (I know we have people building against musl, and Android obviously has bionic.) I just know that not being able to write `target.os == 'Linux'` is going to be confusing. You can imagine my level of interest in GNU/kFreeBSD and GNU/Hurd, but I'm not opposed to supporting them, as long as it's not going to make the much-more-common case of GNU/Linux confusing. :) In any event, that's already in-tree, so let's take it to a followup. > We can certainly invert it. Does it matter? I don't know, it's just very awkward right now. Not the most critical thing, as long as we don't accumulate a lot of cases like this.
Assignee | ||
Comment 23•8 years ago
|
||
https://reviewboard.mozilla.org/r/40751/#review37251 > I don't know, it's just very awkward right now. Not the most critical thing, as long as we don't accumulate a lot of cases like this. You know what? MOZ_VTUNE is only used in js/src, and there is nothing in old-configure that propagated the "implies --enable-profiling" to js/src/old-configure, so in practice, only what is in js/src/old-configure matters. So let's just use that.
Assignee | ||
Comment 24•8 years ago
|
||
https://reviewboard.mozilla.org/r/40751/#review37251 > You know what? MOZ_VTUNE is only used in js/src, and there is nothing in old-configure that propagated the "implies --enable-profiling" to js/src/old-configure, so in practice, only what is in js/src/old-configure matters. So let's just use that. err, "implies --enable-profiling" is what js/src/old-configure did, What old-configure did is the opposite, but the conclusion is still the same.
Assignee | ||
Comment 25•8 years ago
|
||
https://reviewboard.mozilla.org/r/40751/#review37251 > err, "implies --enable-profiling" is what js/src/old-configure did, What old-configure did is the opposite, but the conclusion is still the same. gah, I'm confusing myself again, "implies --enable-profiling" *is* what old-configure did.
Assignee | ||
Comment 26•8 years ago
|
||
https://reviewboard.mozilla.org/r/40751/#review37251 > gah, I'm confusing myself again, "implies --enable-profiling" *is* what old-configure did. Ah, I know what confused me: that the helps says it implies --enable-profiling in js/src/old-configure too.
Assignee | ||
Updated•8 years ago
|
Attachment #8731600 -
Attachment description: MozReview Request: Bug 1257434 - Move --enable-systrace to moz.configure → MozReview Request: Bug 1257434 - Move --enable-systrace to moz.configure. r=ted
Assignee | ||
Comment 27•8 years ago
|
||
Comment on attachment 8731600 [details] MozReview Request: Bug 1257434 - Move --enable-systrace to moz.configure. r=ted Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40739/diff/1-2/
Assignee | ||
Comment 28•8 years ago
|
||
Comment on attachment 8731601 [details] MozReview Request: Bug 1257434 - Move --enable-jprof to moz.configure. r=ted Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40741/diff/1-2/
Attachment #8731601 -
Attachment description: MozReview Request: Bug 1257434 - Move --enable-jprof to moz.configure → MozReview Request: Bug 1257434 - Move --enable-jprof to moz.configure. r=ted
Assignee | ||
Comment 29•8 years ago
|
||
Comment on attachment 8731602 [details] MozReview Request: Bug 1257434 - Move MOZ_ENABLE_PROFILER_SPS to moz.configure. r=ted Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40743/diff/1-2/
Attachment #8731602 -
Attachment description: MozReview Request: Bug 1257434 - Move MOZ_ENABLE_PROFILER_SPS to moz.configure → MozReview Request: Bug 1257434 - Move MOZ_ENABLE_PROFILER_SPS to moz.configure. r=ted
Assignee | ||
Comment 30•8 years ago
|
||
Comment on attachment 8731603 [details] MozReview Request: Bug 1257434 - Move --enable-instruments to moz.configure. r=ted Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40745/diff/1-2/
Attachment #8731603 -
Attachment description: MozReview Request: Bug 1257434 - Move --enable-instruments to moz.configure → MozReview Request: Bug 1257434 - Move --enable-instruments to moz.configure. r=ted
Assignee | ||
Updated•8 years ago
|
Attachment #8731605 -
Attachment description: MozReview Request: Bug 1257434 - Move --enable-callgrind to moz.configure → MozReview Request: Bug 1257434 - Move --enable-callgrind to moz.configure. r=ted
Assignee | ||
Comment 31•8 years ago
|
||
Comment on attachment 8731605 [details] MozReview Request: Bug 1257434 - Move --enable-callgrind to moz.configure. r=ted Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40747/diff/1-2/
Assignee | ||
Comment 32•8 years ago
|
||
Comment on attachment 8731606 [details] MozReview Request: Bug 1257434 - Move --enable-dmd to moz.configure. r=ted Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40749/diff/1-2/
Attachment #8731606 -
Attachment description: MozReview Request: Bug 1257434 - Move --enable-dmd to moz.configure → MozReview Request: Bug 1257434 - Move --enable-dmd to moz.configure. r=ted
Assignee | ||
Comment 33•8 years ago
|
||
Comment on attachment 8731607 [details] MozReview Request: Bug 1257434 - Move --enable-vtune and --enable-profiling to moz.configure. r=ted Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40751/diff/1-2/
Attachment #8731607 -
Attachment description: MozReview Request: Bug 1257434 - Move --enable-vtune and --enable-profiling to moz.configure → MozReview Request: Bug 1257434 - Move --enable-vtune and --enable-profiling to moz.configure. r=ted
Assignee | ||
Updated•8 years ago
|
Attachment #8731608 -
Attachment is obsolete: true
Comment 34•8 years ago
|
||
https://reviewboard.mozilla.org/r/40751/#review37477 That is much simpler! ::: js/moz.configure:105 (Diff revision 2) > +js_option('--enable-profiling', env='MOZ_PROFILING', > + help='Set compile flags necessary for using sampling profilers ' > + '(e.g. shark, perf)') > + > +@depends('--enable-profiling', target) > +def vtune(value, target): I think you mixed up the name of this function and the one below it...
Comment 35•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4f6b5add70 https://hg.mozilla.org/integration/mozilla-inbound/rev/b3f02ffe19e1 https://hg.mozilla.org/integration/mozilla-inbound/rev/029a6dd17e97 https://hg.mozilla.org/integration/mozilla-inbound/rev/52260b41c30b https://hg.mozilla.org/integration/mozilla-inbound/rev/6172dce22b56 https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab651479800 https://hg.mozilla.org/integration/mozilla-inbound/rev/9d3b4b3bc6f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/a5ec589e025b
Comment 36•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/aa4f6b5add70 https://hg.mozilla.org/mozilla-central/rev/b3f02ffe19e1 https://hg.mozilla.org/mozilla-central/rev/029a6dd17e97 https://hg.mozilla.org/mozilla-central/rev/52260b41c30b https://hg.mozilla.org/mozilla-central/rev/6172dce22b56 https://hg.mozilla.org/mozilla-central/rev/1ab651479800 https://hg.mozilla.org/mozilla-central/rev/9d3b4b3bc6f3 https://hg.mozilla.org/mozilla-central/rev/a5ec589e025b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•