Status

defect
RESOLVED FIXED
11 years ago
Last year

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

Tracking

Trunk
mozilla1.9.3a1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Assignee

Description

11 years ago
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

11 years ago
Attachment #341174 - Flags: review?(benjamin) → review-

Comment 1

11 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

11 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?
Assignee

Comment 4

11 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

11 years ago
when would be a good time to land a nuking of --enable-perf-metrics?
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

11 years ago
Is it possible to move stopwatch code to mailnews?
(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

11 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

10 years ago
Lets get this in for next release since I missed the boat for 3.5
Attachment #395667 - Flags: review?(benjamin)

Updated

10 years ago
Attachment #395667 - Flags: review?(benjamin) → review+
Assignee

Comment 11

10 years ago
http://hg.mozilla.org/mozilla-central/rev/092a301bd484
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee: nobody → tglek
Flags: in-testsuite-
OS: Linux → All
Hardware: Other → All
Target Milestone: --- → mozilla1.9.3a1
Depends on: 546103
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+
Attachment #426929 - Flags: review?(bugmail) → review+
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 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

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.