Closed
Bug 457949
Opened 16 years ago
Closed 15 years ago
no need for Stopwatch
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: taras.mozilla, Assigned: taras.mozilla)
References
Details
Attachments
(3 files)
(deleted),
patch
|
benjamin
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Callek
:
review+
asuth
:
review+
|
Details | Diff | Splinter Review |
Stopwatch is only used when MOZ_PERF_METRICS is specified. It's unclear if anyone still uses that. For now I made stopwatch conditional on MOZ_PERF_METRICS being set.
From the CVS log looks like nobody has touched the code in half a decade. Asking Benjamin as he seems to be the person to talk to for abandoned stuff in the tree.
Attachment #341174 -
Flags: review?(benjamin)
Updated•16 years ago
|
Attachment #341174 -
Flags: review?(benjamin) → review-
Comment 1•16 years ago
|
||
Comment on attachment 341174 [details] [diff] [review]
don't compile stopwatch by default
You should stop building stopwatch.cpp by removing it from CPPSRCS in the makefile, rather that ifdefing the entire file.
Assignee | ||
Comment 2•16 years ago
|
||
if we are going to be removing files from compilation, the correct fix would be to not compile libutil unless MOZ_PERF_METRICS is set. Could you hint at how to do that?
Comment 3•16 years ago
|
||
enclose http://mxr.mozilla.org/mozilla-central/source/modules/libutil/src/Makefile.in#45 to #52 in
ifdef MOZ_PERF_METRICS
...
endif
Assignee | ||
Comment 4•16 years ago
|
||
Actually looks like --enable-perf-metrics is broken:
make[3]: Entering directory `/home/taras/work/ff-build.fennec/xulrunner/toolkit/library'
make[3]: *** No rule to make target `-lmozutil_s', needed by `libxul.so'. Stop.
Not sure how long it's been broken for.
Assignee | ||
Comment 5•16 years ago
|
||
when would be a good time to land a nuking of --enable-perf-metrics?
Comment 6•16 years ago
|
||
So, MailNews is now interested in using the stopwatch functionality, tentatively exposed through a thin C++ XPCOM wrapper. How would people feel about changing things up so that MailNews projects (Thunderbird and SeaMonkey) could enable this mechanism and the wrapper? Since this would not affect Firefox (other than the general disabling, but we could avoid changing that for now), we might perchance get it into 1.9.1? (If it can't make 1.9.1, we would need to just copy it into comm-central, barring a better solution to our actual problem.)
The actual problem is a need for the MailNews Global Database (gloda) to be adaptive about its indexing so it doesn't consume 100% of the user's CPU. We can't simply bracket our invocations of functionality with timestamp start/stop's (not that that is a great option) because there is a lot of asynchronous logic happening un-bracketed by javascript (libmime streaming, asynchronous SQLite logic), etc. The prototype implementation using Stopwatch uses the exposed CPU Time and real clock time (samplings spaced by nsITimer events) to estimate Thunderbird's CPU utilization over a reasonable time interval. We set target CPU utilization thresholds based on whether the user is idle (using the idle service) and twiddle our knobs until we are in the target range.
I'm not sure how else I would really go about doing this without using Stopwatch or writing a new cross-platform equivalent, but I'm certainly open to suggestions. Feedback on whether this is desirable in mozilla-central and the best way to do it is appreciated.
Assignee | ||
Comment 7•16 years ago
|
||
Is it possible to move stopwatch code to mailnews?
Comment 8•16 years ago
|
||
(In reply to comment #7)
> Is it possible to move stopwatch code to mailnews?
From my perspective, sure. As long as there's nothing in mozilla-central that needs access to it, it could move into comm-central/mailnews (or some other top-level comm-central directory) without trouble.
Such a strategy also would not need to do anything drastic for 1.9.1; the removal of Stopwatch could take place on the trunk.
The argument against, in my mind, would be that such code has more in common with the platform found in mozilla-central than comm-central. But that's tenuous, as it's hard to imagine what other code would need this functionality. (Especially since other programs would hopefully not be in the boat Thunderbird is in where a lot of things have to be time-shared on the main thread owing to serious single-threaded legacy code.)
Assignee | ||
Comment 9•16 years ago
|
||
(In reply to comment #8)
> (In reply to comment #7)
> > Is it possible to move stopwatch code to mailnews?
>
> From my perspective, sure. As long as there's nothing in mozilla-central that
> needs access to it, it could move into comm-central/mailnews (or some other
> top-level comm-central directory) without trouble.
Sounds good.
> Such a strategy also would not need to do anything drastic for 1.9.1; the
> removal of Stopwatch could take place on the trunk.
It wont happen in 191.
>
> The argument against, in my mind, would be that such code has more in common
> with the platform found in mozilla-central than comm-central. But that's
> tenuous, as it's hard to imagine what other code would need this functionality.
Yeah it is not a good fit for measuring things in firefox. I think we are moving away from having non-firefox components in mozilla central in general.
Assignee | ||
Comment 10•15 years ago
|
||
Lets get this in for next release since I missed the boat for 3.5
Attachment #395667 -
Flags: review?(benjamin)
Updated•15 years ago
|
Attachment #395667 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 11•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Assignee: nobody → tglek
Flags: in-testsuite-
OS: Linux → All
Hardware: Other → All
Target Milestone: --- → mozilla1.9.3a1
Comment 12•15 years ago
|
||
Attachment #426929 -
Flags: review?(bugspam.Callek)
Comment 13•15 years ago
|
||
Comment on attachment 426929 [details] [diff] [review]
(Cv1-CC) Remove MOZ_PERF_METRICS from comm-central too (on 1.9.2+)
[Checkin: Comment 15]
Andrew, the review request here is just to make sure mailnews doesn't need these bits in any "coming" work. (Based on earlier comments). The patch itself is good otherwise.
Attachment #426929 -
Flags: review?(bugspam.Callek)
Attachment #426929 -
Flags: review?(bugmail)
Attachment #426929 -
Flags: review+
Updated•15 years ago
|
Attachment #426929 -
Flags: review?(bugmail) → review+
Comment 14•15 years ago
|
||
Comment on attachment 426929 [details] [diff] [review]
(Cv1-CC) Remove MOZ_PERF_METRICS from comm-central too (on 1.9.2+)
[Checkin: Comment 15]
We migrated/ported nsStopwatch into mailnews/base/util per the proposal in the thread. Remove away!
Comment 15•15 years ago
|
||
Comment on attachment 426929 [details] [diff] [review]
(Cv1-CC) Remove MOZ_PERF_METRICS from comm-central too (on 1.9.2+)
[Checkin: Comment 15]
http://hg.mozilla.org/comm-central/rev/903c24461dd5
Attachment #426929 -
Attachment description: (Cv1) Remove MOZ_PERF_METRICS from comm-central too (on 1.9.2+) → (Cv1-CC) Remove MOZ_PERF_METRICS from comm-central too (on 1.9.2+)
[Checkin: Comment 15]
Updated•15 years ago
|
Blocks: C192ConfSync
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•