Closed Bug 696033 Opened 13 years ago Closed 12 years ago

Add xperf probes

Categories

(Core :: General, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: Yoric, Assigned: Yoric)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files, 22 obsolete files)

17.28 KB, patch
Details | Diff | Splinter Review
8.63 KB, patch
Details | Diff | Splinter Review
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.
I attach a first candidate version of a patch. I have some difficulties testing with xperf, though.
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.
Attachment #569339 - Attachment is obsolete: true
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?
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 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.
Attachment #571944 - Flags: review?(tglek) → review-
(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?
(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.
Attached patch v4. The API. (obsolete) — Splinter Review
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.
Attachment #571944 - Attachment is obsolete: true
Attached patch v4. The Client. (obsolete) — Splinter Review
And now, the corresponding client, with suggestions applied.
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.
Attachment #572558 - Flags: review?(tglek) → review-
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.
Attachment #572559 - Flags: review?(tglek) → review-
Attached patch v5. The API. (obsolete) — Splinter Review
As previously, but without the abstraction, and attempting to force XCode into whitespace submission.
Attachment #572558 - Attachment is obsolete: true
Attached patch v5. The Client. (obsolete) — Splinter Review
As above, without the abstraction. I personally found the previous version more readable, though.
Attachment #572559 - Attachment is obsolete: true
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.
Attachment #574891 - Flags: review?(tglek) → review?(mh+mozilla)
Attachment #574892 - Flags: review?(tglek) → review+
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?
Attachment #574891 - Flags: review?(mh+mozilla) → review-
(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.
Target Milestone: --- → mozilla10
Version: unspecified → Trunk
(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)?
(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.
Target Milestone: mozilla10 → ---
Version: Trunk → unspecified
Attached patch v6. The API. (obsolete) — Splinter Review
New version of the API. Applied all changes except singleton.
Attachment #574891 - Attachment is obsolete: true
Attached patch v6. The Client. (obsolete) — Splinter Review
Propagated changes to client code.
Attachment #574892 - Attachment is obsolete: true
Attachment #576172 - Flags: review?(mh+mozilla)
Attachment #576171 - Flags: review?(mh+mozilla)
Attachment #576172 - Attachment is patch: true
(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?
Also, there was the method names "problem" (them being awkward compared to their description)
Ah, my bad, I will fix this immediately. However, at the moment, I do not understand the naming problem. How would you call them?
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)
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?
We avoid them when instance initialization can fail. Here it never fails.
Attached patch v7. The API. (obsolete) — Splinter Review
Attachment #576171 - Attachment is obsolete: true
Attachment #576171 - Flags: review?(mh+mozilla)
Attachment #576438 - Flags: review?(mh+mozilla)
Attached patch v7. The Client. (obsolete) — Splinter Review
Here we go.
Attachment #576172 - Attachment is obsolete: true
Attachment #576172 - Flags: review?(mh+mozilla)
Attachment #576439 - Flags: review?(mh+mozilla)
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.
Attachment #576438 - Flags: review?(mh+mozilla) → review+
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.
Attachment #576439 - Flags: review?(mh+mozilla) → review+
Attached patch v8. The API. (obsolete) — Splinter Review
By the time the patches land, I really will be able to recite the Coding Guidelines backwards :)
Attachment #576438 - Attachment is obsolete: true
Attachment #576493 - Flags: review?(mh+mozilla)
Attached patch v8. The Client. (obsolete) — Splinter Review
...in Chinese :)
Attachment #576439 - Attachment is obsolete: true
Attachment #576495 - Flags: review?(mh+mozilla)
Attached patch v9. The API. (obsolete) — Splinter Review
This (last?) patch fixes a regression in the API v8.
Attachment #576493 - Attachment is obsolete: true
Attachment #576493 - Flags: review?(mh+mozilla)
Attachment #576525 - Flags: review?(mh+mozilla)
Attachment #576495 - Flags: review?(mh+mozilla) → review+
Attachment #576525 - Flags: review?(mh+mozilla) → review+
Joel, can you try these patches out with your stuff? see if we can get an interleaved log of io + mozilla xperf notications?
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!
Try provider "Mozilla Generic Provider". The events I have defined are "Mozilla Event: Places Init is complete." and ""Mozilla Event: Session Store Window Restored."
"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?
(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.
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>
(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?
(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)
Whiteboard: [waiting on bug 709193]
mozilla-central is now open again (subject to sheriff metering until the backlog clears), adjusting whiteboard accordingly.
Whiteboard: [waiting on bug 709193]
Has this passed try?
(In reply to Ed Morley [:edmorley] from comment #44)
> Has this passed try?

It has, but we are not quite ready to land yet.
Keywords: checkin-needed
Attached patch v10. The Client (obsolete) — Splinter Review
Attachment #576495 - Attachment is obsolete: true
Attached patch v10. The API (obsolete) — Splinter Review
Same patches, but without bitrot.
Attachment #576525 - Attachment is obsolete: true
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/dteller@mozilla.com-84cefd32c756
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/dteller@mozilla.com-5d9b8664e4cd
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/dteller@mozilla.com-90e087d116d3
Attachment #586900 - Attachment is obsolete: true
Attachment #586901 - Attachment is obsolete: true
Attachment #588682 - Attachment is obsolete: true
Attachment #588679 - Attachment is obsolete: true
After a fight with the TryServer, here's the checkin-needed version.
Keywords: checkin-needed
(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!
Keywords: checkin-needed
Attached patch Probe API (obsolete) — Splinter Review
Attachment #588683 - Attachment is obsolete: true
Attached patch API Client (obsolete) — Splinter Review
Here is the merged version. Thanks!
Attachment #588684 - Attachment is obsolete: true
Whiteboard: [autoland-try]
Whiteboard: [autoland-try] → [autoland-in-queue]
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
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/autolanduser@mozilla.com-1574c1dc0019
Whiteboard: [autoland-in-queue]
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 :-)
Keywords: checkin-needed
Attachment #594305 - Attachment is obsolete: true
Attachment #594308 - Attachment is obsolete: true
(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?
Keywords: checkin-needed
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
Status: NEW → ASSIGNED
Keywords: checkin-needed
Target Milestone: --- → mozilla13
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/ccaccc29ae9e
https://hg.mozilla.org/mozilla-central/rev/0eebc33d8593
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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
(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?
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
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 :-)
Thanks. If this patch is the culprit, I can make instantiation of probes depend on either an environment variable or a preference.
if there's a regression in MED, we should also see a regression in the MAX test. Did anyone check graphs yet?
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.
Depends on: 726354
You need to log in before you can comment on or make changes to this bug.