Open Bug 745864 Opened 12 years ago Updated 2 years ago

Add a bunch of SAMPLE_LABELS for profiling fennec

Categories

(Core :: General, defect)

x86_64
Linux
defect

Tracking

()

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Details

(Whiteboard: [leave open])

Attachments

(5 files, 1 obsolete file)

Attached patch Graphics labelsSplinter Review
      No description provided.
Attachment #615408 - Flags: review?(bgirard)
Attached patch Layout labelsSplinter Review
Attachment #615409 - Flags: review?(roc)
Comment on attachment 615408 [details] [diff] [review]
Graphics labels

LGTM. You should take a look with mac OMTC where we have stackwalking to get an idea of the slow places on desktop. Make sure we hit all the good spots.
Attachment #615408 - Flags: review?(bgirard) → review+
Attached patch Android labelsSplinter Review
Attachment #615412 - Flags: review?(bgirard)
Attached patch event labelsSplinter Review
Whiteboard: [autoland-try]
Whiteboard: [autoland-try] → [autoland-in-queue]
Autoland Patchset:
	Patches: 615408, 615409, 615412, 615413
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=0b7a20007d97
Try run started, revision 0b7a20007d97. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=0b7a20007d97
Comment on attachment 615412 [details] [diff] [review]
Android labels

SAMPLE_LABEL don't nest within the same function because of the RAII macro. At best you'll get a visibility warning.
Attachment #615412 - Flags: review?(bgirard)
Attachment #615421 - Flags: review?(bgirard) → review+
Attachment #615412 - Flags: review+
Comment on attachment 615409 [details] [diff] [review]
Layout labels

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

What's the overhead of each label invocation?
Try run for 0b7a20007d97 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0b7a20007d97
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-0b7a20007d97
Whiteboard: [autoland-in-queue]
Attachment #615408 - Flags: approval-mozilla-central?
Attachment #615412 - Flags: approval-mozilla-central?
Comment on attachment 615421 [details] [diff] [review]
make SAMPLE_LABELS per line instead of per independent scope

Causes compile errors:
/home/bgirard/mozilla/kiwifox/tree/gfx/layers/basic/BasicLayers.cpp:570: error: 'sampler_raii' was not declared in this scope
/home/bgirard/mozilla/kiwifox/tree/gfx/layers/basic/BasicLayers.cpp:570: error: expected ',' or ';' before '(' token
/home/bgirard/mozilla/kiwifox/tree/gfx/layers/basic/BasicLayers.cpp: In member function 'bool mozilla::layers::BasicLayerManager::EndTransactionInternal(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, const nsIntRegion&, const nsIntRegion&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags)':
/home/bgirard/mozilla/kiwifox/tree/gfx/layers/basic/BasicLayers.cpp:1535: error: 'sampler_raii' was not declared in this scope
/home/bgirard/mozilla/kiwifox/tree/gfx/layers/basic/BasicLayers.cpp:1535: error: expected ',' or ';' before '(' token
Attachment #615421 - Flags: review+ → review-
Attachment #615421 - Attachment is obsolete: true
Attachment #615845 - Flags: review?(jmuizelaar)
Attachment #615845 - Flags: review?(jmuizelaar)
Attachment #615845 - Flags: review+
Attachment #615845 - Flags: approval-mozilla-central?
Attachment #615408 - Flags: approval-mozilla-central? → approval-mozilla-central+
Attachment #615412 - Flags: approval-mozilla-central? → approval-mozilla-central+
Attachment #615845 - Flags: approval-mozilla-central? → approval-mozilla-central+
I talked to BenWa on irc, let's take these patches now, but follow up quickly with a patch to disable these labels in release configs
Attachment #615845 - Flags: checkin+
Attachment #615412 - Flags: checkin+
Attachment #615408 - Flags: checkin+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline April 15-18) from comment #8)
> Comment on attachment 615409 [details] [diff] [review]
> Layout labels
> 
> Review of attachment 615409 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What's the overhead of each label invocation?

It varies from platform to platform but basically it's a tls lookup
three null checks, a couple of loads, two comparisons, an increment, a decrement and 4 stores. Also, it will obviously cause some additional register pressure in the caller.
Comment on attachment 615409 [details] [diff] [review]
Layout labels

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

I'm not super-thrilled about adding that overhead, but OK.

Couldn't we have a SAMPLE_LABEL_MAIN_THREAD that we use when we know we're on the main thread, avoiding the TLS lookup at least?
Attachment #615409 - Flags: review?(roc) → review+
Attachment #615409 - Flags: approval-mozilla-central+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline April 22-25) from comment #16)
> Comment on attachment 615409 [details] [diff] [review]
> Layout labels
> 
> Review of attachment 615409 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not super-thrilled about adding that overhead, but OK.
> 
> Couldn't we have a SAMPLE_LABEL_MAIN_THREAD that we use when we know we're
> on the main thread, avoiding the TLS lookup at least?

We decided we would implement that with a DEBUG time assert to make sure that piece of code is always executed in the main thread.
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.