106551 - nsITimelineService needs work

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Suresh Duddi (gone)]

The following needs to be added
- NS_TIMELINE_RESET_TIMER
- NS_TIMELINE_MARK_TIMER needs to take a text that can be printed along with the
timer output.

Comment 1

[Suresh Duddi (gone)]

Also making the output be controlled by PRLog mechanism and going to a file
would be good.

Comment 2

[Samir Gehani]

Attachment: Reset timer implementation.

Comment 3

[Samir Gehani]

Already have r=dp.  Someone want to give me an sr for the reset patch (id=57659)?

Comment 4

[Suresh Duddi (gone)]

Attachment: Implements ability to display user strings with MarkTimer. Used by load library marks.

This patch adds the ability for MarkTimer to take a user string that will get
displayed in the output. PR_LoadLibrary marks use this to display which dll is
being loaded.

Comment 5

[Suresh Duddi (gone)]

Simon, sr= ? for the two patches here. Doug/Samir, wanna r= the MarkTimer patch.

I would like to get this in as early as possible as it aids in perf analysis -
dlls loaded

Comment 6

[Simon Fraser [no longer active]]

Comment on attachment 57659 [details] [diff] [review]
Reset timer implementation.

sr=sfraser

Comment 7

[Simon Fraser [no longer active]]

It might be slightly better to have the macro supply the nsnull:

#define NS_TIMELINE_MARK_TIMER(timerName) NS_TimelineMarkTimer(timerName, nsnull)

rather than using C++ default values. Either way, sr=sfraser on the second patch.

Comment 8

[Suresh Duddi (gone)]

Comment on attachment 58075 [details] [diff] [review]
Implements ability to display user strings with MarkTimer. Used by load library marks.

Sr. Simon says yes.

Comment 9

[Samir Gehani]

Comment on attachment 58075 [details] [diff] [review]
Implements ability to display user strings with MarkTimer. Used by load library marks.

r=sgehani

Simon, for the mac, could we pass the C string representation of
mTargetSpec.name to NS_TIMELINE_MARK_TIMER1().?

Comment 10

[Simon Fraser [no longer active]]

Sure. You'd need Pascal-to-C string conversion, but that would be good.

Comment 11

[Suresh Duddi (gone)]

So would I do something like : (I confess. Pascal was 12 years ago)

#ifdef MOZ_TIMELINE
char *p = mTargetSpec.name;
int n = *p++;
char buf[128];
strncpy(buf, p, n);
buf[n] = '\0';
NS_TIMELINE_MARK_TIMER1("PR_LoadLibrary", buf);
#endif

Comment 12

[Samir Gehani]

If PLStringsFuncs is available you can use that.  If not here's one implementation:
<http://lxr.mozilla.org/seamonkey/source/xpfe/bootstrap/appleevents/nsMacUtils.cpp#144>

Comment 13

[Suresh Duddi (gone)]

Attachment: NS_TIMELINE_MARK_FUNCTION

Facility to time functions

Comment 14

[Simon Fraser [no longer active]]

+class nsFnTime {

Ugly name  :)

+public:
+    char mTimer[64];
+    PRBool mMark;
+    nsFnTime(const char *timer, PRBool mark) : mMark(mark)

How about defaulting mark to PR_FALSE?

+    {
+        strcpy(mTimer, timer);

Hoo, you'd better be sure they passed a string < 64 bytes long. Maybe a strncpy?

Comment 15

[Suresh Duddi (gone)]

Attachment: NS_TIMELINE_MARK_FUNCTION

Turns out that I cant use a buffer for timername as these are added into the
hash table and hence needs to be around forever :-) So I dont use a buffer.
This is the way all of timeline is designed. We should change that if we feel
the need enmass.

Rest of simon's comments taken. I put the default for mark as TRUE.

Comment 16

[Samir Gehani]

Comment on attachment 58550 [details] [diff] [review]
NS_TIMELINE_MARK_FUNCTION

r=sgehani

Comment 17

[Simon Fraser [no longer active]]

Minor nit. You can change:

+#define NS_TIMELINE_MARK_FUNCTION(timer) nsFunctionTimer functionTimer(timer, 1 
/* mark */)
+#define NS_TIMELINE_TIME_FUNCTION(timer) nsFunctionTimer functionTimer(timer, 0 
/* no mark, only time */)

to

+#define NS_TIMELINE_MARK_FUNCTION(timer) nsFunctionTimer functionTimer(timer) /* 
mark */
+#define NS_TIMELINE_TIME_FUNCTION(timer) nsFunctionTimer functionTimer(timer, 
PR_FALSE) /* no mark, only time */

sr=sfraser

Comment 18

[Suresh Duddi (gone)]

sfraser - sure will do.

Comment 19

[Suresh Duddi (gone)]

Attachment: Runtime disable of timeline; NS_TIMELINE_MARK_FUNCTION1

Adds support for NS_TIMELINE_MARK_FUNCTION1 that takes an additional string.

Also NS_TIMELINE_DISABLE will disable timeline at runtime. This can be used to
see if eliminate any skew that timeline causes in overall start time
measurement.

Comment 20

[Simon Fraser [no longer active]]

Comment on attachment 59176 [details] [diff] [review]
Runtime disable of timeline; NS_TIMELINE_MARK_FUNCTION1

+    if (LL_IS_ZERO(initTime)) {
+	 TimelineInit();

Where did initTime come from? Oh, look, it's a global! It would be nice to
rename the globals to have names like gInitTime etc.

sr=sfraser either way.

Comment 21

[Samir Gehani]

Comment on attachment 59176 [details] [diff] [review]
Runtime disable of timeline; NS_TIMELINE_MARK_FUNCTION1

Change:
>+int g_timelineDisabled = PR_FALSE;

to:
PRBool g_timelineDisabled = PR_FALSE;

r=sgehani

Comment 22

[Suresh Duddi (gone)]

samir - did your proposed change.
simon - there are a few more globals. Thought I would change their names enmass.
So didnt do it now.

thanks for review guys.

Comment 23

[Suresh Duddi (gone)]

All the patches have been checked in.