Closed
Bug 633239
Opened 14 years ago
Closed 13 years ago
event loop responsiveness for Android
Categories
(Core Graveyard :: Widget: Android, defect)
Tracking
(fennec+)
RESOLVED
FIXED
mozilla10
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: ted, Assigned: blassey)
References
Details
(Whiteboard: [inbound])
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
Splitting this off from bug 606574 because a) we need this for Firefox first anyway, and b) I don't know the Android widget code well enough to quickly put together an implementation.
Reporter | ||
Comment 1•14 years ago
|
||
The implementation consists of a C++ function, "bool FireAndWaitForTracerEvent()". It should submit an event to the native event queue and then block until the event is procesed. The GTK2 implementation is here, for example:
https://bugzilla.mozilla.org/attachment.cgi?id=511406&action=diff#a/widget/src/gtk2/WidgetTraceEvent.cpp_sec1
Updated•14 years ago
|
Assignee: nobody → crowderbt
Assignee | ||
Updated•14 years ago
|
Assignee: crowderbt → blassey.bugs
Assignee | ||
Comment 2•14 years ago
|
||
Attachment #525750 -
Flags: review?(ted.mielczarek)
Attachment #525750 -
Flags: review?(doug.turner)
Comment 3•14 years ago
|
||
i'd like to see this working before r+. it looks like it should work.
fireAndWaitForTracerEvent is called on some thread. We must ensure that it is never called on the main handler thread or we will deadlock. Can we add assertion -- or a runtime abort if that happens?
Ted, how do we test this? It looks pretty interesting!
Reporter | ||
Comment 4•14 years ago
|
||
FireAndWaitForTracerEvent is always called from a particular background thread. It's documented in the header that it must not be called from the UI thread:
http://hg.mozilla.org/mozilla-central/file/81ea8b39ed4e/widget/public/WidgetTraceEvent.h#l43
I think that's enough, although a main-thread check with a runtime abort would be a fine sanity check addition (although I didn't do this in any of the other widget implementations).
You can test this by running the browser with MOZ_INSTRUMENT_EVENT_LOOP=1 in the environment. On Android, you'll also want to set MOZ_INSTRUMENT_EVENT_LOOP_OUTPUT to a path to a file to contain the output (it defaults to stdout). You should get at least a start and stop line in the file, and presumably some other lines if you interact with the UI and get some slowness. There's some slightly more useful documentation in the source:
http://hg.mozilla.org/mozilla-central/file/81ea8b39ed4e/toolkit/xre/EventTracer.cpp#l38
Note that my patches in bug 606574 got backed out for leaking, so you'll have to either re-apply them or apply your patch on top of their original changesets to test. I should be able to get them re-landed today or tomorrow.
Reporter | ||
Comment 5•14 years ago
|
||
Comment on attachment 525750 [details] [diff] [review]
patch
I need to update my patches on bug 606574, I'll come back to this when I'm done just in case I have to change the API at all. Should get back to this early next week.
Reporter | ||
Comment 6•13 years ago
|
||
Comment on attachment 525750 [details] [diff] [review]
patch
Looks fine, but my revised patches added init and cleanup methods, so you'll need to put stubs of those in: bool InitWidgetTracing() and void CleanUpWidgetTracing() (both in namespace mozilla).
Attachment #525750 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Updated•13 years ago
|
Whiteboard: [needs-review (dougt)]
Comment 7•13 years ago
|
||
Comment on attachment 525750 [details] [diff] [review]
patch
per ted, this needs more (the init and close methods). Also, a sample output would be nice to see.
Attachment #525750 -
Flags: review?(doug.turner) → review-
Assignee | ||
Updated•13 years ago
|
tracking-fennec: --- → ?
Updated•13 years ago
|
Whiteboard: [needs-review (dougt)]
Assignee | ||
Updated•13 years ago
|
tracking-fennec: ? → 8+
Whiteboard: [needs-review (dougt)]
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #525750 -
Attachment is obsolete: true
Attachment #550617 -
Flags: review?(doug.turner)
Assignee | ||
Updated•13 years ago
|
tracking-fennec: 8+ → +
Updated•13 years ago
|
tracking-fennec: + → 9+
Comment 9•13 years ago
|
||
I'm very interested in using this patch with the profiler I'm suggesting in bug 633239.
Comment 10•13 years ago
|
||
Comment on attachment 550617 [details] [diff] [review]
patch
Review of attachment 550617 [details] [diff] [review]:
-----------------------------------------------------------------
good to have the patch lying around, but I am not sure we should land any of this until there is some user needing this. Ted - what is the status here? Is this going to be part of some automated testing ?
Reporter | ||
Comment 11•13 years ago
|
||
Alice is spinning up Talos testing of this metric in bug 631571. Note that all the other platform backends landed months ago. I don't see any reason why Android shouldn't do the same.
Updated•13 years ago
|
tracking-fennec: 9+ → +
Updated•13 years ago
|
Attachment #550617 -
Flags: review?(doug.turner) → review+
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #550617 -
Attachment is obsolete: true
Attachment #569207 -
Flags: review?(ted.mielczarek)
Reporter | ||
Comment 13•13 years ago
|
||
Comment on attachment 569207 [details] [diff] [review]
patch
Review of attachment 569207 [details] [diff] [review]:
-----------------------------------------------------------------
::: widget/src/android/AndroidBridge.cpp
@@ +1086,5 @@
> + void SignalTracerThread()
> + {
> + MutexAutoLock lock(*mTracerLock);
> + mHasRun = PR_TRUE;
> + mTracerCondVar->Notify();
This...isn't a member function of TracerRunnable, so this won't work.
Attachment #569207 -
Flags: review?(ted.mielczarek) → review-
Assignee | ||
Comment 14•13 years ago
|
||
Attachment #569207 -
Attachment is obsolete: true
Attachment #569754 -
Flags: review?(ted.mielczarek)
Reporter | ||
Updated•13 years ago
|
Attachment #569754 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 15•13 years ago
|
||
Whiteboard: [needs-review (dougt)] → [inbound]
Comment 16•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•