696033 - Add xperf probes

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Under Windows, we are currently using XPerf to trace time-consuming operations. To make sense of these results, we need custom probes informing us of key points, in particular end of launch, start of shutdown.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Possibly one way of adding xperf probes

I attach a first candidate version of a patch. I have some difficulties testing with xperf, though.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Second version. Linking issues for the moment.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: Third version. Seems to work, testing in progress.

Ok, I believe that I have finally managed to crack that nut. I attach a patch, but for the sake of posterity, here are a few of the key technical points that caused most grief.


1/ Despite what some of the MSDN documentation seems to imply, you cannot setup probes fully from code, even for trivial probes. You also need to declare your probes in a .mof file that must be run through mofcomp.exe.

To ensure that your file was compiled correctly, launch "xperf -providers R" as administrator and look for your probe provider. For this patch, the provider is called "Mozilla Generic Provider".


2/ Your code will fail with a vague error number ("improper handle") if xperf is not launched with the correct arguments. You can only launch xperf once step 1/ is complete. To launch xperf, don't attempt to read the documentation (at first), it is vague and misleading. Rather, launch

xperf -start my_friendly_neighborhood_tracing_session -on "Mozilla Generic Provider"


3/ In the Windows API, registration of your probes produces a value of type TRACEHANDLE. Triggering a probe requires a value of type TRACEHANDLE. They are absolutely unrelated. The first one is a value whose sole role is to permit unregistration. To get the second one, you must call function GetTraceLoggerHandle. However, you cannot call this function directly, as it is invalid in most contexts. To call this function, you must first provide a callback and only this callback is allowed to call the function and set, by side effect, the corresponding TRACEHANDLE.


4/ Oh, yeah, probably not really specific to this specific API, but someone at Microsoft decided that it would be fun to alias just about everything to `long long unsigned int` – this includes both in pointers to data structures, out pointers to pointers to data structures, error values, etc. This is immensely fun to debug.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

this is great stuff.  My only concern is if we want to collect this in the tinderbox runs we will have to manually configure all the machines (we are talking many weeks or a couple months).  Just something to keep in mind :)  

If we define our 'Mozilla Generic Provider' now and set it up, can we adjust the providers and data that is available automatically?

Comment 5

[David Teller [:Yoric] - still alive but not very active]

I don't see any reason for which we couldn't keep using this "Mozilla Generic Provider" (or at least its GUID, if we don't like the name). Unfortunately, each probe also requires its own registration - see "probes.mof" in the attached patch. So I guess we should first decide which probes we are going to need.

Comment 6

[(dormant account)]

Comment on attachment 571944 [details] [diff] [review]
Third version. Seems to work, testing in progress.

Your indendation is wrong when you modify existing code, it's also wrong in new files. See https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide
We don't indent namespaces, typically use 2space tabs, etc.

Don't add superfluous newlines
mozilla:: should never be used from within our code.

typedef mozilla::whatever whatever; when needed.

probes.h should probably be called mozxperf.h and live in xpcom/glue. Break up the patch into implementation and users for followups.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #6)
> probes.h should probably be called mozxperf.h and live in xpcom/glue. 

I'm ok with everything else but I don't understand that specific sentence. What would this do in glue?

Comment 8

[(dormant account)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #7)
> (In reply to Taras Glek (:taras) from comment #6)
> > probes.h should probably be called mozxperf.h and live in xpcom/glue. 
> 
> I'm ok with everything else but I don't understand that specific sentence.
> What would this do in glue?

It's a location to put stuff that is used throughout our tree. I expect it to be useful outside of startup/ directory.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: v4. The API.

Here's the API. After a discussion with glandium, we decided to put it in xpcom/build and not in xpcom/glue, as it is a citizen of libxul rather than a citizen of the executable.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: v4. The Client.

And now, the corresponding client, with suggestions applied.

Comment 11

[(dormant account)]

Comment on attachment 572558 [details] [diff] [review]
v4. The API.

This patch was much easier to read than the previous one, thanks for the cleanup.

> 		nsXPCOMStrings.cpp \
> 		Services.cpp \
> 		Omnijar.cpp \
>+    mozperfprobe.cpp \
> 		$(NULL)

watch your indent.

Lets get rid of abstraction here. Make this conditionally built on windows only. Ifdef away users.

I think the probe manager should be a singleton, with AddProbe() registering it and returning a handle to the user that can be triggered.

Comment 12

[(dormant account)]

Comment on attachment 572559 [details] [diff] [review]
v4. The Client.

>+//{A3DA04E0-57D7-482A-A1C1-61DA5F95BACB}
>+#define NS_PLACES_INIT_COMPLETE_EVENT_CID \
>+  { 0xA3DA04E0, 0x57D7, 0x482A, \
>+  {0xA1, 0xC1, 0x61, 0xDA, 0x5F, 0x95, 0xBA, 0xCB} }
>+#define NS_SESSION_STORE_WINDOW_RESTORED_EVENT_CID \
>+  { 0x917B96B1, 0xECAD, 0x4DAB, \
>+  {0xA7, 0x60, 0x8D, 0x49, 0x02, 0x77, 0x48, 0xAE} }
>+
>+

One newline is enough.

>+static NS_DEFINE_CID(kApplicationTracingCID,         NS_APPLICATION_TRACING_CID);
>+static NS_DEFINE_CID(kPlacesInitCompleteCID,         NS_PLACES_INIT_COMPLETE_EVENT_CID);
>+static NS_DEFINE_CID(kSessionStoreWindowRestoredCID, NS_SESSION_STORE_WINDOW_RESTORED_EVENT_CID);
>+                     
> using namespace mozilla;
> extern PRTime gXRE_mainTimestamp;
> // The following tracks our overhead between reaching XRE_main and loading any XUL
> extern PRTime gCreateTopLevelWindowTimestamp;// Timestamp of the first call to
>                                              // nsAppShellService::CreateTopLevelWindow
> extern PRTime gFirstPaintTimestamp;
> // mfinklesessionstore-browser-state-restored might be a better choice than the one below
> static PRTime gRestoredTimestamp = 0;       // Timestamp of sessionstore-windows-restored
>@@ -154,32 +178,52 @@ nsAppStartup::Init()
> 
>   NS_TIME_FUNCTION_MARK("Got AppShell service");
> 
>   nsCOMPtr<nsIObserverService> os =
>     mozilla::services::GetObserverService();
>   if (!os)
>     return NS_ERROR_FAILURE;
> 
>+

Please watch your whitespace changes.
>+    rv = mProbesManager->MakeProbe(kPlacesInitCompleteCID,
>+                                   NS_LITERAL_CSTRING("Places initialization complete"),
>+                                   getter_AddRefs(mPlacesInitCompleteProbe));
>+    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Could not initialize system probe for 'places'");
>+    
>+    rv = mProbesManager->MakeProbe(kSessionStoreWindowRestoredCID,
>+                                   NS_LITERAL_CSTRING("Session Windows restored"),
>+                                   getter_AddRefs(mSessionWindowRestoredProbe));
>+    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Could not initialize system probe for 'session windows'");
>+    
>+    rv = mProbesManager->StartSession();
This API looks good. Minor nit, might as well use our observer strings for descriptions. Those are unique and can be looked up in the docs.

>     gRestoredTimestamp = PR_Now();
>+    if(mSessionWindowRestoredProbe)
>+		mSessionWindowRestoredProbe->Trigger();
>+  } else if (!strcmp(aTopic, "places-init-complete")) {
>+	  if(mPlacesInitCompleteProbe)
>+		mPlacesInitCompleteProbe->Trigger();
>   } else {
>     NS_ERROR("Unexpected observer topic.");
>   }
> 

Again, watch your whitespace.


Overall this patch is mostly good. Just need to watch that xcode doesn't chew up your source code. r-ing it based on whitespace mainly.

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: v5. The API.

As previously, but without the abstraction, and attempting to force XCode into whitespace submission.

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: v5. The Client.

As above, without the abstraction. I personally found the previous version more readable, though.

Comment 15

[(dormant account)]

Comment on attachment 574891 [details] [diff] [review]
v5. The API.

Sorry didn't have time to review this today. Mike should be able to finish this.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 574891 [details] [diff] [review]
v5. The API.

Review of attachment 574891 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/build/Makefile.in
@@ +68,5 @@
>  		Omnijar.cpp \
>  		$(NULL)
>  
> +ifeq ($(OS_ARCH),WINNT)
> +CPPSRCS += 		mozperfprobe.cpp

Remove these whitespaces.

@@ +129,5 @@
>    Omnijar.h \
>    $(NULL)
>  
> +ifeq ($(OS_ARCH),WINNT)
> +EXPORTS += mozperfprobe.h

Drop the moz from the filename. I also think it would be better to have the namespace name and the file name be similar. This also means the file should be in EXPORTS_mozilla, which further means the file will be included with #include "mozilla/filename.h" in other parts of the code.

::: xpcom/build/mozperfprobe.cpp
@@ +49,5 @@
> +namespace mozilla {
> +namespace probes {
> +
> +#if defined(MOZ_LOGGING)
> +PRLogModuleInfo* gProbeLog = PR_NewLogModule("SysProbe");

make it static.

Style nit: we don't have a firm coding style wrt references and pointers AFAIK, but we most usually use type *ptr instead of type* ptr, and type &ref instead of type& ref.

@@ +98,5 @@
> +  event.Class.Level   = TRACE_LEVEL_INFORMATION;
> +  
> +  ULONG result        = TraceEvent(mManager->mSessionHandle, &event);
> +  
> +#if defined(MOZ_LOGGING)

You don't need this #ifdef. LOG is going to expand to nothing already.

@@ +147,5 @@
> +  mSessionHandle  = NULL;
> +  mRegistrationHandle = NULL;
> +  mApplicationUID = applicationUID;
> +  mApplicationName= applicationName;
> +  mInitialized    = true;

Remove the extra whitespaces

@@ +178,5 @@
> +        ULONG result = GetLastError();
> +        LOG(("Probes: ControlCallback failed, %ul", result));
> +        return result;
> +      } else if(context -> mIsActive && context -> mSessionHandle
> +                && context -> mSessionHandle != sessionHandle) {

Why are there spaces around "->" ?

@@ +214,5 @@
> +}
> +
> +nsresult ProbeManager::StartSession()
> +{
> +	return StartSession(mAllProbes);

Remove the extra whitespaces.

@@ +223,5 @@
> +  const size_t probesCount = aProbes.Length();
> +  NS_ENSURE_TRUE(mInitialized, NS_ERROR_FAILURE);
> +  _TRACE_GUID_REGISTRATION* probes = new _TRACE_GUID_REGISTRATION[probesCount];
> +  for(unsigned int i = 0; i < probesCount; ++i) {
> +    const Probe*      probe = aProbes[i];

Remove the extra whitespaces.

@@ +225,5 @@
> +  _TRACE_GUID_REGISTRATION* probes = new _TRACE_GUID_REGISTRATION[probesCount];
> +  for(unsigned int i = 0; i < probesCount; ++i) {
> +    const Probe*      probe = aProbes[i];
> +    const Probe* probeX = static_cast<const Probe*>(probe);
> +    probes[i].Guid = (LPCGUID)&( probeX->mGUID );

No spaces in parenthesis.

::: xpcom/build/mozperfprobe.h
@@ +61,5 @@
> +namespace mozilla {
> +namespace probes {
> +  
> +#if defined(MOZ_LOGGING)
> +extern PRLogModuleInfo* gProbeLog;

You don't need that in the header.

@@ +143,5 @@
> +   * behavior is unspecified.
> +   */
> +  static nsresult Make(const nsCID& applicationUID,
> +                       const nsACString& applicationName,
> +                       ProbeManager** aOutProbeManager);

Function name seems awkward compared to the function description.

@@ +165,5 @@
> +   * behavior is undefined.
> +   */
> +  nsresult MakeProbe(const nsCID& eventUID,
> +                     const nsACString& eventName,
> +                     Probe** aOutProbe);

Likewise.

For both functions, since basically it will only succeed or fail, and you probably don't really care how it failed, just make the functions return the value or NULL instead of nsresult. If you need refcounting, return an already_AddRefed<type> and have the function do NS_IF_ADDREF(value) before returning it (or if you use an nsCOMPtr or nsRefPtr in the function, return ptr.forget()).

Is there a reason you would want to create more than one probe manager? If not, why is it not a singleton as Taras asked in comment 11?

Comment 17

[David Teller [:Yoric] - still alive but not very active]

(In reply to Mike Hommey [:glandium] from comment #16)
> Comment on attachment 574891 [details] [diff] [review] [diff] [details] [review]
> v5. The API.

Ok, will fix.

> Drop the moz from the filename.

The moz came from a previous review (comment 6).

> Style nit: we don't have a firm coding style wrt references and pointers
> AFAIK, but we most usually use type *ptr instead of type* ptr, and type &ref
> instead of type& ref.

Ok, I will have to get used to that.


> Why are there spaces around "->" ?
[...]
> No spaces in parenthesis.

I personally find this more readable. Is this somehow incompatible with our coding guidelines? 

> For both functions, since basically it will only succeed or fail, and you
> probably don't really care how it failed, just make the functions return the
> value or NULL instead of nsresult. If you need refcounting, return an
> already_AddRefed<type> and have the function do NS_IF_ADDREF(value) before
> returning it (or if you use an nsCOMPtr or nsRefPtr in the function, return
> ptr.forget()).

Ok, I will do that.

> Is there a reason you would want to create more than one probe manager? If
> not, why is it not a singleton as Taras asked in comment 11?

Well, the underlying API allows up to 1000 distinct probes manager or so in one process, (although it suggests only using a few) so I did not see much point in enforcing a singleton. If you feel that this improves the quality, I can change this behavior, though.

Comment 18

[Mike Hommey [:glandium]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> (In reply to Mike Hommey [:glandium] from comment #16)
> > Comment on attachment 574891 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > v5. The API.
> 
> Ok, will fix.
> 
> > Drop the moz from the filename.
> 
> The moz came from a previous review (comment 6).

Well, if you put these things in the mozilla namespace, and export in mozilla/ (which is where new stuff should end up), then moz should be dropped.

> > Why are there spaces around "->" ?
> [...]
> > No spaces in parenthesis.
> 
> I personally find this more readable. Is this somehow incompatible with our
> coding guidelines? 

I don't think there are strict coding guidelines for this (not that I'm aware of), but i know of no place in the codebase where the coding style is with spaces around "->" and inside parenthesis.

> > Is there a reason you would want to create more than one probe manager? If
> > not, why is it not a singleton as Taras asked in comment 11?
> 
> Well, the underlying API allows up to 1000 distinct probes manager or so in
> one process, (although it suggests only using a few) so I did not see much
> point in enforcing a singleton. If you feel that this improves the quality,
> I can change this behavior, though.

That the underlying API allows that is nice, but the question is more whether this mozilla API needs to do the same. Is there an incentive to having different callers create their own manager, as opposed to having a singleton possibly handling several OS-level probe managers (or just use one, for that matter)?

Comment 19

[David Teller [:Yoric] - still alive but not very active]

(In reply to Mike Hommey [:glandium] from comment #18)
> > The moz came from a previous review (comment 6).
> 
> Well, if you put these things in the mozilla namespace, and export in
> mozilla/ (which is where new stuff should end up), then moz should be
> dropped.

Ok.

> > > Why are there spaces around "->" ?
> > [...]
> > > No spaces in parenthesis.
> > 
> > I personally find this more readable. Is this somehow incompatible with our
> > coding guidelines? 
> 
> I don't think there are strict coding guidelines for this (not that I'm
> aware of), but i know of no place in the codebase where the coding style is
> with spaces around "->" and inside parenthesis.

Ok, complying.

> > > Is there a reason you would want to create more than one probe manager? If
> > > not, why is it not a singleton as Taras asked in comment 11?
> > 
> > Well, the underlying API allows up to 1000 distinct probes manager or so in
> > one process, (although it suggests only using a few) so I did not see much
> > point in enforcing a singleton. If you feel that this improves the quality,
> > I can change this behavior, though.
> 
> That the underlying API allows that is nice, but the question is more
> whether this mozilla API needs to do the same. Is there an incentive to
> having different callers create their own manager, as opposed to having a
> singleton possibly handling several OS-level probe managers (or just use
> one, for that matter)?

By definition of the underlying OS mechanisms, creating/launching a probe manager requires entering some data on the manager in an external Manifest Object Format file. According to the documentation, creating/launching a probe requires entering additional data on the probes in the _same_ MOF file – I have not checked whether the "same file" policy is actually enforced, but the syntax of MOF files seems to suggest it.

Consequently, if we want the client code to be able to specify the probes (which seems pretty much the only reason to divide between client and server code), we also need to give control on the probe manager to the client. I believe that this would be quite in contradiction with the singleton approach.

Additional arguments may apply if we decide to use various probe managers for e10s.

For these reasons, I advocate against the singleton design.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Attachment: v6. The API.

New version of the API. Applied all changes except singleton.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Attachment: v6. The Client.

Propagated changes to client code.

Comment 22

[Mike Hommey [:glandium]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> > For both functions, since basically it will only succeed or fail, and you
> > probably don't really care how it failed, just make the functions return the
> > value or NULL instead of nsresult. If you need refcounting, return an
> > already_AddRefed<type> and have the function do NS_IF_ADDREF(value) before
> > returning it (or if you use an nsCOMPtr or nsRefPtr in the function, return
> > ptr.forget()).
> 
> Ok, I will do that.

You didn't. Oversight?

Comment 23

[Mike Hommey [:glandium]]

Also, there was the method names "problem" (them being awkward compared to their description)

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Ah, my bad, I will fix this immediately. However, at the moment, I do not understand the naming problem. How would you call them?

Comment 25

[Mike Hommey [:glandium]]

Actually, I'd be tempted to say Make should just be a constructor, and MakeProbe either CreateProbe or GetProbe.

While at it, another nit, for forward use of classes, we tend to put a forward declaration ahead and then use the type name directly instead of prepending it with "class" each time (like you do for ProbeManager)

Comment 26

[David Teller [:Yoric] - still alive but not very active]

MakeProbe vs. CreateProbe/GetProbe, no problem. Do we have guidelines on such method naming schemes?

As for turning "Make" into a constructor, this is how I would design the class myself in some other settings, but I thought we attempted to avoid constructors?

Comment 27

[Mike Hommey [:glandium]]

We avoid them when instance initialization can fail. Here it never fails.

Comment 28

[David Teller [:Yoric] - still alive but not very active]

Attachment: v7. The API.

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Attachment: v7. The Client.

Here we go.

Comment 30

[Mike Hommey [:glandium]]

Comment on attachment 576438 [details] [diff] [review]
v7. The API.

Review of attachment 576438 [details] [diff] [review]:
-----------------------------------------------------------------

100% style nits. Other than that it looks good. I'll just add a few comments other than those inline:
- we tend to put spaces after if, for, while.
- please put a commit message in the patch, as well as user/committer information, that will help when landing.

::: xpcom/build/perfprobe.cpp
@@ +134,5 @@
> +                           const nsACString &aApplicationName):
> +  mSessionHandle(NULL),
> +  mRegistrationHandle(NULL),
> +  mApplicationUID(aApplicationUID),
> +  mApplicationName(aApplicationName){

We tend to do
 : member(value)
 , member2(value2)
{
}

when using multiple lines. Further additions only modify one line, this way.

@@ +213,5 @@
> +  const size_t probesCount = aProbes.Length();
> +  _TRACE_GUID_REGISTRATION* probes = new _TRACE_GUID_REGISTRATION[probesCount];
> +  for(unsigned int i = 0; i < probesCount; ++i) {
> +    const Probe* probe = aProbes[i];
> +    const Probe* probeX = static_cast<const Probe*>(probe);

"type *" instead of "type* "

@@ +220,5 @@
> +  ULONG result =
> +    RegisterTraceGuids(&ControlCallback
> +                     /*RequestAddress: Sets mSessions appropriately.*/,
> +                     this
> +                     /*RequestContext: Pased to ControlCallback*/,

Typo

::: xpcom/build/perfprobe.h
@@ +90,5 @@
> +  /**
> +   * The system GUID associated to this probe. See the documentation
> +   * of |ProbeManager::Make| for more details.
> +   */
> +  const GUID          mGUID;

spaces.

@@ +185,5 @@
> +protected:
> +  /**
> +   * `true` if a session is in activity, `false` otherwise.
> +   */
> +  bool              mIsActive;

spaces.

@@ +230,5 @@
> +};
> +}
> +};
> +
> +#endif

We tend to have comments on #endifs and brackets closing namespaces refering to what is being closed.

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 576439 [details] [diff] [review]
v7. The Client.

Review of attachment 576439 [details] [diff] [review]:
-----------------------------------------------------------------

Style nits, mostly. r+ with these addressed.

::: toolkit/components/startup/nsAppStartup.cpp
@@ +104,5 @@
> +/**
> + * Events sent to the system for profiling purposes
> + */
> +//An arbitrary GUID, used by the OS to differentiate sources
> +//509962E0-406B-46F4-99BA-5A009F8D2225

If these GUIDs need to be synchronized with the mof file, it would be better to add a comment about it.
Add spaces after //, and put the GUID between brackets. Or without brackets, as you wish. Either way, make it use the same style as the GUID below.

@@ +112,5 @@
> +
> +//{A3DA04E0-57D7-482A-A1C1-61DA5F95BACB}
> +#define NS_PLACES_INIT_COMPLETE_EVENT_CID \
> +  { 0xA3DA04E0, 0x57D7, 0x482A, \
> +  {0xA1, 0xC1, 0x61, 0xDA, 0x5F, 0x95, 0xBA, 0xCB} }

Spacing differs from the CID above.

@@ +115,5 @@
> +  { 0xA3DA04E0, 0x57D7, 0x482A, \
> +  {0xA1, 0xC1, 0x61, 0xDA, 0x5F, 0x95, 0xBA, 0xCB} }
> +#define NS_SESSION_STORE_WINDOW_RESTORED_EVENT_CID \
> +  { 0x917B96B1, 0xECAD, 0x4DAB, \
> +  {0xA7, 0x60, 0x8D, 0x49, 0x02, 0x77, 0x48, 0xAE} }

This one doesn't have a comment with the GUID in "string form".

@@ +193,5 @@
>    os->AddObserver(this, "sessionstore-windows-restored", true);
>    os->AddObserver(this, "profile-change-teardown", true);
>    os->AddObserver(this, "xul-window-registered", true);
>    os->AddObserver(this, "xul-window-destroyed", true);
> +  os->AddObserver(this, "places-init-complete", true);

Why not under #if defined(XP_WIN) ?

::: xpcom/build/perfprobe.h
@@ +222,5 @@
>     * `true` if initialization has been performed, `false` until then.
>     */
>    bool              mInitialized;
>    
> +  friend Probe;//Needs to access |mSessionHandle|

Changes to this file should be in the other patch.

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Attachment: v8. The API.

By the time the patches land, I really will be able to recite the Coding Guidelines backwards :)

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Attachment: v8. The Client.

...in Chinese :)

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Attachment: v9. The API.

This (last?) patch fixes a regression in the API v8.

Comment 35

[(dormant account)]

Joel, can you try these patches out with your stuff? see if we can get an interleaved log of io + mozilla xperf notications?

Comment 36

[Joel Maher ( :jmaher ) (UTC -8)]

how do I turn these on?  do I need to register a specific provider?  I assume I would need to adjust these values:
http://hg.mozilla.org/build/talos/file/5ae66b11330f/talos/xperf.config#l168

I could probably get to this later in the week, lets just figure out what we need to do!

Comment 37

[David Teller [:Yoric] - still alive but not very active]

Try provider "Mozilla Generic Provider". The events I have defined are "Mozilla Event: Places Init is complete." and ""Mozilla Event: Session Store Window Restored."

Comment 38

[Joel Maher ( :jmaher ) (UTC -8)]

"c:\perftools\xperf.exe" -on PROC_THREAD+LOADER+HARD_FAULTS+FILENAME+FILE_IO+FILE_IO_INIT+"Mozilla Generic Provider" -stackwalk FileRead+FileWrite+FileFlush+"Mozilla Event: Places Init is complete." -MaxBuffers 1024 -BufferSize 1024 -f test.etl

xperf: error: -stackwalk: Unknown flag 'Mozilla' (line 1, character 30)
xperf: error: -stackwalk: at: FileRead+FileWrite+FileFlush+Mozilla Event: Places
 Init is complete.
xperf: error: -stackwalk:                                  ^


This doesn't work.  Is there a way that you tested this?  Maybe there are providers and stackwalk variables that have no spaces?

Comment 39

[David Teller [:Yoric] - still alive but not very active]

(In reply to Joel Maher (:jmaher) from comment #38)
> This doesn't work.  Is there a way that you tested this?  Maybe there are
> providers and stackwalk variables that have no spaces?

I do not think that you need to stackwalk at all to access our "Mozilla Event" probes – at least, they show up in the .etl file without any stackwalk command-line argument. For my testing, I did

xperf -start my_friendly_neighborhood_tracing_session -on "Mozilla Generic Provider"

and then xperfview to see the results.

However, if you need to remove spaces, you can change the name of the probes by altering the .mof file. Also, do not forget that you need to "compile" that .mof file on the host system, to register the probes.

Comment 40

[Joel Maher ( :jmaher ) (UTC -8)]

Is there something we need to setup on the windows machine before accessing this:
C:\Users\mozilla>"c:\perftools\xperf.exe" -on PROC_THREAD+LOADER+HARD_FAULTS+FIL
ENAME+FILE_IO+FILE_IO_INIT+"Mozilla Generic Provider" -MaxBuffers 1024 -BufferSi
ze 1024 -f test.etl
xperf: error: NT Kernel Logger: Invalid flags. (0x3ec).

C:\Users\mozilla>xperf -start my_friendly_neighborhood_tracing_session -on "Mozi
lla Generic Provider"
xperf: error: my_friendly_neighborhood_tracing_session: Invalid flags. (0x3ec).

C:\Users\mozilla>

Comment 41

[David Teller [:Yoric] - still alive but not very active]

(In reply to Joel Maher (:jmaher) from comment #40)
> Is there something we need to setup on the windows machine before accessing
> this:
> C:\Users\mozilla>"c:\perftools\xperf.exe" -on
> PROC_THREAD+LOADER+HARD_FAULTS+FIL
> ENAME+FILE_IO+FILE_IO_INIT+"Mozilla Generic Provider" -MaxBuffers 1024
> -BufferSi
> ze 1024 -f test.etl
> xperf: error: NT Kernel Logger: Invalid flags. (0x3ec).

Have you run the mof compiler?

Comment 42

[Ed Morley [:emorley]]

(Marking whiteboard just to make it easier to keep track of which bugs in the checkin-needed search cannot be landed due to bug 709193)

Comment 43

[Ed Morley [:emorley]]

mozilla-central is now open again (subject to sheriff metering until the backlog clears), adjusting whiteboard accordingly.

Comment 44

[Ed Morley [:emorley]]

Has this passed try?

Comment 45

[David Teller [:Yoric] - still alive but not very active]

(In reply to Ed Morley [:edmorley] from comment #44)
> Has this passed try?

It has, but we are not quite ready to land yet.

Comment 46

[David Teller [:Yoric] - still alive but not very active]

Attachment: v10. The Client

Comment 47

[David Teller [:Yoric] - still alive but not very active]

Attachment: v10. The API

Same patches, but without bitrot.

Comment 48

[Mozilla RelEng Bot]

Try run for 84cefd32c756 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=84cefd32c756
Results (out of 14 total builds):
    exception: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-84cefd32c756

Comment 49

[Mozilla RelEng Bot]

Try run for 5d9b8664e4cd is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5d9b8664e4cd
Results (out of 14 total builds):
    exception: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5d9b8664e4cd

Comment 50

[Mozilla RelEng Bot]

Try run for 90e087d116d3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=90e087d116d3
Results (out of 14 total builds):
    success: 9
    warnings: 2
    failure: 3
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-90e087d116d3

Comment 51

[David Teller [:Yoric] - still alive but not very active]

Attachment: try: -b do -p all -u none -t none --post-to-bugzilla

Comment 52

[David Teller [:Yoric] - still alive but not very active]

Attachment: Client API

Comment 53

[David Teller [:Yoric] - still alive but not very active]

Attachment: Probe API

Comment 54

[David Teller [:Yoric] - still alive but not very active]

Attachment: Client API

Comment 55

[David Teller [:Yoric] - still alive but not very active]

After a fight with the TryServer, here's the checkin-needed version.

Comment 56

[Scott Johnson (:jwir3)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #55)
> After a fight with the TryServer, here's the checkin-needed version.

David:

These patches don't apply cleanly. The probe API applies with some minor tweaking to xpcom/build/Makefile.in, which I was able to fix, but the client API seems to need substantial changes to toolkit/components/startup/nsAppStartup.cpp.

Could you fix these and then repost the patches? 

Thanks!

Comment 57

[David Teller [:Yoric] - still alive but not very active]

Attachment: Probe API

Comment 58

[David Teller [:Yoric] - still alive but not very active]

Attachment: API Client

Here is the merged version. Thanks!

Comment 59

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 594305, 594308
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/rev/1574c1dc0019
Try run started, revision 1574c1dc0019. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=1574c1dc0019

Comment 60

[Mozilla RelEng Bot]

Try run for 1574c1dc0019 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1574c1dc0019
Results (out of 207 total builds):
    success: 184
    warnings: 22
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1574c1dc0019

Comment 61

[Ed Morley [:emorley]]

Please could you tweak your hgrc to automatically add author info (guide here: https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F) + add a commit message when attaching patches, since it makes pushing half a dozen checkin-neededs a lot easier :-)

Comment 62

[David Teller [:Yoric] - still alive but not very active]

Attachment: Add xperf probes (the API);

Comment 63

[David Teller [:Yoric] - still alive but not very active]

Attachment: Add xperf probes (sample startup probes);

Comment 64

[David Teller [:Yoric] - still alive but not very active]

(In reply to Ed Morley [:edmorley] from comment #61)
> Please could you tweak your hgrc to automatically add author info (guide
> here:
> https://developer.mozilla.org/en/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F) + add a commit message when attaching patches, since it makes
> pushing half a dozen checkin-neededs a lot easier :-)

Sorry about that. Is this better?

Comment 65

[Ed Morley [:emorley]]

That's great - thank you :-)

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccaccc29ae9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0eebc33d8593

Comment 66

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/ccaccc29ae9e
https://hg.mozilla.org/mozilla-central/rev/0eebc33d8593

Comment 67

[Ed Morley [:emorley]]

Could this be the cause of:
Regression :( Ts Shutdown, MED Dirty Profile increase 19.8% on XP Mozilla-Inbound-Non-PGO

https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tree-management/RdLAPxfvCpM

Comment 68

[David Teller [:Yoric] - still alive but not very active]

(In reply to Ed Morley [:edmorley] from comment #67)
> Could this be the cause of:
> Regression :( Ts Shutdown, MED Dirty Profile increase 19.8% on XP
> Mozilla-Inbound-Non-PGO
> 
> https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tree-
> management/RdLAPxfvCpM

That's what I fear. How could I find out whether this measure is noise or signal?

Comment 69

[Mike Hommey [:glandium]]

I was going to suggest to trigger more ts runs on the push with these patches landed, but they don't appear on tbpl except on android...
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0eebc33d8593

Comment 70

[Ed Morley [:emorley]]

I've retriggered dep builds on 0eebc33d8593 and the push prior, so hopefully we'll have something to use tbpl compare against in a couple of hours :-)

Comment 71

[David Teller [:Yoric] - still alive but not very active]

Thanks. If this patch is the culprit, I can make instantiation of probes depend on either an environment variable or a preference.

Comment 72

[Marco Bonardo [:mak]]

if there's a regression in MED, we should also see a regression in the MAX test. Did anyone check graphs yet?

Comment 73

[David Teller [:Yoric] - still alive but not very active]

I double-checked my code. For the moment, I tend to believe that it could not cause a regression at shutdown when tracing is not activated (which should be the case). I'm waiting for tbpl's results, though.