Open
Bug 745864
Opened 12 years ago
Updated 2 years ago
Add a bunch of SAMPLE_LABELS for profiling fennec
Categories
(Core :: General, defect)
Tracking
()
NEW
People
(Reporter: jrmuizel, Assigned: jrmuizel)
Details
(Whiteboard: [leave open])
Attachments
(5 files, 1 obsolete file)
5.77 KB,
patch
|
BenWa
:
review+
blassey
:
approval-mozilla-central+
BenWa
:
checkin+
|
Details | Diff | Splinter Review |
8.56 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
1.27 KB,
patch
|
BenWa
:
review+
blassey
:
approval-mozilla-central+
BenWa
:
checkin+
|
Details | Diff | Splinter Review |
1.70 KB,
patch
|
Details | Diff | Splinter Review | |
1.80 KB,
patch
|
jrmuizel
:
review+
blassey
:
approval-mozilla-central+
BenWa
:
checkin+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #615408 -
Flags: review?(bgirard)
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #615409 -
Flags: review?(roc)
Comment 2•12 years ago
|
||
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+
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #615412 -
Flags: review?(bgirard)
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Whiteboard: [autoland-try]
Updated•12 years ago
|
Whiteboard: [autoland-try] → [autoland-in-queue]
Comment 5•12 years ago
|
||
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 6•12 years ago
|
||
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)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #615421 -
Flags: review?(bgirard)
Updated•12 years ago
|
Attachment #615421 -
Flags: review?(bgirard) → review+
Updated•12 years ago
|
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?
Comment 9•12 years ago
|
||
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
Updated•12 years ago
|
Whiteboard: [autoland-in-queue]
Assignee | ||
Updated•12 years ago
|
Attachment #615408 -
Flags: approval-mozilla-central?
Assignee | ||
Updated•12 years ago
|
Attachment #615412 -
Flags: approval-mozilla-central?
Comment 10•12 years ago
|
||
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-
Comment 11•12 years ago
|
||
Attachment #615421 -
Attachment is obsolete: true
Attachment #615845 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•12 years ago
|
Attachment #615845 -
Flags: review?(jmuizelaar)
Attachment #615845 -
Flags: review+
Attachment #615845 -
Flags: approval-mozilla-central?
Updated•12 years ago
|
Attachment #615408 -
Flags: approval-mozilla-central? → approval-mozilla-central+
Updated•12 years ago
|
Attachment #615412 -
Flags: approval-mozilla-central? → approval-mozilla-central+
Updated•12 years ago
|
Attachment #615845 -
Flags: approval-mozilla-central? → approval-mozilla-central+
Comment 12•12 years ago
|
||
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
Comment 13•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/473904bdde30 https://hg.mozilla.org/integration/mozilla-inbound/rev/8e191b7164b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/078f22017c69
Whiteboard: keep-open
Updated•12 years ago
|
Attachment #615845 -
Flags: checkin+
Updated•12 years ago
|
Attachment #615412 -
Flags: checkin+
Updated•12 years ago
|
Attachment #615408 -
Flags: checkin+
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/473904bdde30 https://hg.mozilla.org/mozilla-central/rev/8e191b7164b4 https://hg.mozilla.org/mozilla-central/rev/078f22017c69
Assignee: nobody → jmuizelaar
Whiteboard: keep-open → [leave open]
Assignee | ||
Comment 15•12 years ago
|
||
(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+
Updated•12 years ago
|
Attachment #615409 -
Flags: approval-mozilla-central+
Comment 18•12 years ago
|
||
(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.
But it's not implemented yet?
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•