Closed
Bug 508518
Opened 15 years ago
Closed 14 years ago
Implement nsUITimerCallback with one-shot timer
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0b3+ | --- |
People
(Reporter: romaxa, Assigned: smaug)
References
Details
Attachments
(4 files, 5 obsolete files)
8.72 KB,
patch
|
Details | Diff | Splinter Review | |
9.89 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
9.75 KB,
patch
|
Details | Diff | Splinter Review | |
9.71 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
Currently nsUITimer in nsEventStateManager Notifying every 5 seconds... This is a bit problematic for mobile devices... Probably it would be better to create user-active/inactive notification based on ONE_SHOT timer.
Reporter | ||
Comment 1•15 years ago
|
||
Attachment #392687 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 2•15 years ago
|
||
Comment on attachment 392687 [details] [diff] [review] Proposed fix with one shot timer. So what guarantees that cycle collector is called often enough? nsJSEnvironment.cpp relies on user-interaction-XXX messages. Maybe this approach could work, if something was added to cycle collector. It could fire some notification if there are X number new suspected objects and nsJSEnvironment.cpp could listen also for that notification.
Attachment #392687 -
Flags: review?(Olli.Pettay) → review-
Assignee | ||
Comment 3•15 years ago
|
||
Perhaps cycle collector could check when it was last time called, and if longer than X seconds ago and there are let's say 100 new suspected objects, it could call collect (asynchronously). That sort of duplicates some of the functionality of nsJSEnvironment :/
Reporter | ||
Comment 4•15 years ago
|
||
Do you mean CC triggering collection "ONLY" when user-inactive notification is coming? I think that is very bad... I think it should start one_shot timer (~5 sec) after new objects added, and then do collection by timer if user is not active...
Reporter | ||
Comment 5•15 years ago
|
||
I guess it would be enough to call MaybeCC(PR_TRUE); from nsJSContext::ScriptEvaluated and invoke CC while there are no User activity but, some JS activity available...
Assignee | ||
Comment 6•15 years ago
|
||
(In reply to comment #4) > Do you mean CC triggering collection "ONLY" when user-inactive notification is > coming? I think that is very bad... No. CC is delayed a bit if user is active, but will run eventually. But in any case, currently nsJSEnvironment.cpp depends on user-XXX notifications firing every X seconds. > I think it should start one_shot timer (~5 sec) after new objects added, and > then do collection by timer if user is not active... There may be no "new objects". It is just possible that something has become garbage which can be collected. Can you still explain why it is problematic that the timer fires every 5s. Firing an event shouldn't take too much cpu time and if there is nothing to collect, cycle collector isn't called. Do you basically want that gecko doesn't do anything when things are idle? If that is the case, something somewhere must detect when things aren't idle anymore. That something may not be user-interactivity, but something else. Cycle collector can pretty easily detect if there are new suspected objects. (See nsCycleCollector_suspectedCount())
Assignee | ||
Comment 7•15 years ago
|
||
(In reply to comment #5) > I guess it would be enough to call MaybeCC(PR_TRUE); from > nsJSContext::ScriptEvaluated That is not enough. It is possible that JS doesn't do anything but C++ creates cycle collectable garbage.
Reporter | ||
Comment 8•15 years ago
|
||
>Can you still explain why it is problematic that the timer fires every 5s.
>Firing an event shouldn't take too much cpu time and if there is nothing to
>collect, cycle collector isn't called.
Ok, problem is next:
1) We have slow swap.
When browser engine in swap it should stay there, but these timers are waking up engine all the time, it causing extra swap activity.
2) This is mobile device, and applications should not do any extra activity without good reason.
In Gecko (at least in our stripped embedding version of microb) I have found ~5 such timers:
UICallback - 5 sec
JS Watchdog - 1 sec (Fixed)
nsUpdateService - depends from pref
nsPlacesDBFlusher - 2 min
nsNavHistory - 5 min
Necko connection manager PruneDead connections ~15 min
Also when mobile system in idle, it is better to stay with not collected data, and do CC check when we get back from idle
Reporter | ||
Comment 9•15 years ago
|
||
which C++ code can produce collectable garbage? is there are some central place where objects added? can we put there are some handlers for CC startup?
Assignee | ||
Comment 10•15 years ago
|
||
Any code can add new cycle collectable objects. Whatever timers run and execute something, or callbacks which handle some network activity, or ...
Reporter | ||
Comment 11•15 years ago
|
||
(In reply to comment #10) > Any code can add new cycle collectable objects. Whatever timers run and execute > something, or callbacks which handle some network activity, or ... Yes, I understand, but I believe that all that collectable objects pointers stored into some storage (list, hash...) Can we just hook that place where new objects marked as collectable, and do some logic there?
Assignee | ||
Comment 12•15 years ago
|
||
Cycle collector has a list of suspected objects.
Assignee | ||
Comment 13•15 years ago
|
||
Romaxa, will you perhaps provide a new patch?
Reporter | ||
Comment 14•15 years ago
|
||
(In reply to comment #13) > Romaxa, will you perhaps provide a new patch? I'll try.
Reporter | ||
Comment 15•15 years ago
|
||
I think I have some problems, can you point me which function is adding new participants to cycle-collector? I want to trigger CC after some amount of objects added to CC...
Assignee | ||
Comment 16•15 years ago
|
||
See how new objects are added to mPurpleBuf in cycle collector.
Comment 17•14 years ago
|
||
This is should be set to block bug 583135
Comment 18•14 years ago
|
||
Reacting to system going idle is still under debate how it will be handled. One thing is for sure that fast started browser should not cause wake-ups bug 572329. For system going idle there are basically two options for browser: suspend whole browser and handle this so that system wont get confused about this(basically modified bug 572329) or suspend all timers and activity so that energy consumption is as low as it can be. Later seems to be most popular at the moment. It could be that there needs to be two separate notifications about idle. First would be "soft idle" that would mean that user not interacting with browser anymore and second would be "full idle" meaning that no activity is allowed from browser anymore. With this bug I'm not sure how we can do this "full idle" if browser is not completely suspended. Olli would solution for "soft idle" be that timer is suspended when notification of this "soft idle" is received then cc is done once and after that only after some amount of suspected objects is exceeded?
Assignee | ||
Comment 19•14 years ago
|
||
The problem is to detect "amount of suspected objects is exceeded" in a fast way. One possibility is to use Romaxa's patch, but also add check for suspected object counts to nsJSContext::ScriptEvaluated and call CC if there are lots of new objects. Not perfect but might work, and shouldn't regress AddRef/Release performance.
Comment 20•14 years ago
|
||
How about the approach used to fix the JS watchdog timer - if there is any C++ activity in the last X seconds, continue firing the timer at regular intervals, otherwise suspend it until activity occurs. Whether there is any C++ activity can be detected by noting the time of last activity in the relevant places, which for C++ is PR_Sleep, condition variable stuff, etc. Something very similar to detecting when C++ is active is done in http://mozakai.blogspot.com/2010/09/tools-for-debugging-wakeups.html Alternatively, there are platform-dependent tricks that might help. Maybe listening to SIGALRM?
Updated•14 years ago
|
tracking-fennec: --- → ?
Comment 22•14 years ago
|
||
Is the new cycle collector mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=564118#c17 going to have effect for this implementation?
Comment 23•14 years ago
|
||
(In reply to comment #19) > The problem is to detect "amount of suspected objects is exceeded" in a fast > way. > > One possibility is to use Romaxa's patch, but also add check for suspected > object counts to nsJSContext::ScriptEvaluated and call CC if there are lots > of new objects. Not perfect but might work, and shouldn't regress > AddRef/Release > performance. smaug, won't that only work for JS garbage, and not stuff created in C++, which I thought was a concern based on the discussion before? If that isn't a problem, then let's go with that. Alternatively, the only way I can think of to detect C++ activity would be to add something to C++ code that must run, if cycle collection is relevant. For example, * In NSPR: Add a callback that happens every so often, if threads are active. * In malloc/free. Might not work if nothing is malloced/freed, but something does become collectable. On the other hand that case isn't so bad - if the browser is idle, and no new memory is being allocated, might as well wait for activity in order to do cycle collection.
Assignee | ||
Comment 24•14 years ago
|
||
(In reply to comment #23) > smaug, won't that only work for JS garbage, and not stuff created in C++, which > I thought was a concern based on the discussion before? It would work for C++ garbage, but only if there is some JS code running. (For page loads etc. CC runs anyway.) So it might work reasonable well. Or might not. > * In NSPR: Add a callback that happens every so often, if threads are active. This might work. Need to be careful if this causes performance problems. > * In malloc/free. Might not work if nothing is malloced/freed, but something > does become collectable. This is pretty often the case. And we certainly don't want to slow down malloc/free at all. They show up high enough in the profiles already now. Not performance-vice, but otherwise good solution would be to add something to cycle collectable objects' AddRef/Release, but those are extremely performance critical, so we must not make then slower.
Comment 25•14 years ago
|
||
Hmm, it would be nice to find higher-level stuff, and not malloc or AddRef etc., which as you say are very performance sensitive. So, as for high-level stuff, how about TimerThread's main loop, maybe also event loop stuff (nsIAppShell)? That is, if X seconds pass in those loops, call the collector. If idle, do nothing. Would that work, or is it possible to have significant activity without hitting these?
Assignee | ||
Comment 26•14 years ago
|
||
(In reply to comment #25) > So, as for high-level stuff, how about TimerThread's main loop, maybe also > event loop stuff (nsIAppShell)? That is, if X seconds pass in those loops, call > the collector. If idle, do nothing. Something like that could work. After x seconds pass in those loops and/or y timer/runnable runs check if there are lots of new suspected objects and if there are, run CC.
Comment 27•14 years ago
|
||
WIP patch, causes some orange I need to figure out. But before I spend time on that, I'd like to know if I'm in the right direction. The patch removes the timer, and instead sends the active/inactive notifications from places which are active when C++ code is active: The TimerThread main loop, and the threads' ProcessNextEvent. I tried to optimize the code (see PR_AtomicSet in the patch), so I'm hoping it won't have a perf impact.
Attachment #392687 -
Attachment is obsolete: true
Attachment #479255 -
Flags: feedback?(Olli.Pettay)
Updated•14 years ago
|
tracking-fennec: ? → 2.0b2+
Updated•14 years ago
|
Assignee: nobody → azakai
Assignee | ||
Comment 28•14 years ago
|
||
Comment on attachment 479255 [details] [diff] [review] TimerThread & ProcessNextEvent update user activity >+void >+nsEventStateManager::DoUpdateUserActivity() >+{ >+ // Do the actual action on the main thread, because the observer >+ // service should be used only there >+ class UpdateEvent : public nsRunnable { { should be in the next line. Please, no classes inside a method. They are just ugly, IMHO. >+ PRUint32 *mPreviousActivityCount; >+ >+ public: >+ UpdateEvent(PRUint32 *previousActivityCount) : >+ mPreviousActivityCount(previousActivityCount) { } >+ >+ NS_IMETHOD run() { { should be in the next line. But does this actually work? The method name should be Run, not run. >+ static void CheckUpdateUserActivity() >+ { >+ mozilla::TimeStamp now = mozilla::TimeStamp::Now(); >+ if (mLastActivityNoticeTime.IsNull() || >+ (now - mLastActivityNoticeTime).ToMilliseconds() >= >+ NS_USER_INTERACTION_INTERVAL) { >+ // Don't use locks in order to do the first check - that needs to be very >+ // very fast. >+ PRInt32 old = PR_AtomicSet(&mUpdatingUserActivity, 1); Hmm, so >+ if (old == 0) { >+ if (!mLastActivityNoticeTime.IsNull()) >+ DoUpdateUserActivity(); >+ mLastActivityNoticeTime = now; >+ mUpdatingUserActivity = 0; >+ printf("Update! %d\n", PR_Now()); >+ } >+ } >+ } >+ >+private: >+ static void DoUpdateUserActivity(); >+ >+ static mozilla::TimeStamp mLastActivityNoticeTime; >+ static PRInt32 mUpdatingUserActivity; >+ static PRUint32 mPreviousActivityCount; > }; > >diff --git a/xpcom/threads/TimerThread.cpp b/xpcom/threads/TimerThread.cpp >--- a/xpcom/threads/TimerThread.cpp >+++ b/xpcom/threads/TimerThread.cpp >@@ -44,16 +44,17 @@ > #include "nsAutoLock.h" > #include "nsThreadUtils.h" > #include "pratom.h" > > #include "nsIObserverService.h" > #include "nsIServiceManager.h" > #include "nsIProxyObjectManager.h" > #include "mozilla/Services.h" >+#include "nsEventStateManager.h" No go. Don't make xpcom depend on something very gecko internal. > > #include <math.h> > > NS_IMPL_THREADSAFE_ISUPPORTS2(TimerThread, nsIRunnable, nsIObserver) > > TimerThread::TimerThread() : > mInitInProgress(0), > mInitialized(PR_FALSE), >@@ -302,16 +303,20 @@ NS_IMETHODIMP TimerThread::Run() > // refcount of 1 with an armed timer (a timer whose only reference > // is from the timer thread) and when it hits this will remove the > // timer from the timer thread and thus destroy the last reference, > // preventing this situation from occurring. > NS_ASSERTION(rc != 0, "destroyed timer off its target thread!"); > } > timer = nsnull; > >+ // This loop is one of the places where we have constant activity, so it's >+ // a good place to check for user activity >+ nsEventStateManager::CheckUpdateUserActivity(); >+ Actually, why do we need anything in timerthread? The main thread should be enough.
Attachment #479255 -
Flags: feedback?(Olli.Pettay) → feedback-
Comment 29•14 years ago
|
||
Asking bz to also take a look, because of the xpcom/threads/nsThreadUtilsInternal.h changes, as based on the source comments I believe he wrote that file.
Summary of patch and motivation:
* Allow multiple global thread observers. (Changes to nsThreadUtilsInternal.h and nsThread.cpp.)
* nsEventStateManager adds one such observer. The observer is called each time a thread processes an event, and tells nsEventStateManager to check whether to send user activity/inactivity events. That way, we do not need a timer for this, which would run constantly; this way we only run if there is actual activity in the program, so we save power.
smaug, about:
>Actually, why do we need anything in timerthread? The main thread should be
enough.
Good point - removed the code from TimerThread in this patch. So we just check when threads process events. Note that it's for all threads, not just the main one, but I think we need that - it's possible in theory to have just side threads active, I presume?
Attachment #479255 -
Attachment is obsolete: true
Attachment #479601 -
Flags: review?(bzbarsky)
Attachment #479601 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 30•14 years ago
|
||
But cycle collector works only in main thread and cycle collectable objects aren't thread safe.
Comment 31•14 years ago
|
||
Comment on attachment 479601 [details] [diff] [review] Patch v2 I did not know that...
Attachment #479601 -
Attachment is obsolete: true
Attachment #479601 -
Flags: review?(bzbarsky)
Attachment #479601 -
Flags: review?(Olli.Pettay)
Comment 32•14 years ago
|
||
Fixed patch. Everything now done correctly on the main thread.
Attachment #479931 -
Flags: review?(bzbarsky)
Attachment #479931 -
Flags: review?(Olli.Pettay)
Comment 33•14 years ago
|
||
So other than cycle collector, what else observes these notifications? I assume you audited the consumers?
Comment 34•14 years ago
|
||
(In reply to comment #33) > So other than cycle collector, what else observes these notifications? I > assume you audited the consumers? Yeah, the cycle collector is the only thing listening for them. Hmm, perhaps we should change their names while we are at it? How about 'system-active-user-active' and 'system-active-user-inactive' (to make explicit that we care about user activity, or not, only when the system/browser is active)?
Assignee | ||
Comment 35•14 years ago
|
||
So this takes romaxa's patch and adds CCIfUserInactive call to nsXPConnect::AfterProcessNextEvent so that cycle collector is called occasionally even if user is inactive. bent may have some ideas how to improve this all.
Updated•14 years ago
|
Attachment #479931 -
Flags: review?(bzbarsky)
Comment 36•14 years ago
|
||
(In reply to comment #35) > Created attachment 480641 [details] [diff] [review] > alternative approach > > So this takes romaxa's patch and adds CCIfUserInactive call to > nsXPConnect::AfterProcessNextEvent so that cycle collector is called > occasionally even if user is inactive. > The UI timer stops firing when there is no user interaction but activity still occurs - for example, when animated GIFs are being shown. So garbage can be generated but not collected in that case?
Comment 37•14 years ago
|
||
Comment on attachment 479931 [details] [diff] [review] Patch v3 Please ignore my previous comment, I misunderstood smaug's patch. I am in favor of going with smaug's patch.
Attachment #479931 -
Attachment is obsolete: true
Attachment #479931 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 38•14 years ago
|
||
Jst, what do you think of this approach. Or should bent look at this. I posted the patch to tryserver to get perf results. Locally the patch works reasonable well when we create garbage in a setTimeout loop, and yet it prevents running CC at all if there isn't anything really happening.
Assignee: azakai → Olli.Pettay
Attachment #480699 -
Flags: review?(jst)
Assignee | ||
Comment 39•14 years ago
|
||
Attachment #480699 -
Attachment is obsolete: true
Attachment #480758 -
Flags: review?(jst)
Attachment #480699 -
Flags: review?(jst)
Assignee | ||
Comment 40•14 years ago
|
||
I couldn't see any regressions in the tryserver, and I've been using this now a day and seems to work quite nicely.
Comment 41•14 years ago
|
||
Comment on attachment 480758 [details] [diff] [review] yet another null check for xpcshell tests Looks good.
Attachment #480758 -
Flags: review?(jst) → review+
Assignee | ||
Comment 42•14 years ago
|
||
Unfortunately this may caused dromaeo DOM regression. I backed out. I'll push to tryserver with a bit smaller NS_MIN_INACTIVE_SUSPECT_CHANGES.
Comment 43•14 years ago
|
||
Maybe I'm looking at it backwards, but it seems like it improved dromaeo_dom, not worsened it... http://perf.snarkfest.net/compare-talos/index.html?oldRevs=1f8d775d79b1&newRev=602b5dd047a6&tests=%20a11y,tdhtml,tdhtml_nochrome,tp4,tp4_memset,tp4_pbytes,tp4_rss,tp4_shutdown,tp4_xres,dromaeo_basics,dromaeo_css,dromaeo_dom,dromaeo_jslib,dromaeo_sunspider,dromaeo_v8,tsspider,tsspider_nochrome,tgfx,tgfx_nochrome,tscroll,tsvg,tsvg_opacity,ts,ts_cold,ts_cold_generated_max,ts_cold_generated_max_shutdown,ts_cold_generated_med,ts_cold_generated_med_shutdown,ts_cold_generated_min,ts_cold_generated_min_shutdown,ts_cold_shutdown,ts_places_generated_max,ts_places_generated_max_shutdown,ts_places_generated_med,ts_places_generated_med_shutdown,ts_places_generated_min,ts_places_generated_min_shutdown,ts_shutdown,twinopen&submit=true
Assignee | ||
Comment 44•14 years ago
|
||
That seems to compare dromaeo in the wrong way. In dromaeo higher numbers are better, in many other tests lower numbers are better.
Comment 45•14 years ago
|
||
> I'll push to tryserver with a bit smaller NS_MIN_INACTIVE_SUSPECT_CHANGES.
Did you manage to get rid of the perf regression?
Assignee | ||
Comment 46•14 years ago
|
||
No. Changing CC scheduling is tricky.
Comment 47•14 years ago
|
||
If we need to wait for this to be "right", we can push out of beta 2.
tracking-fennec: 2.0b2+ → 2.0b3+
Assignee | ||
Comment 48•14 years ago
|
||
I'm going to push few different versions of the patch to tryserver this weekend to see if there is some value for NS_MIN_INACTIVE_SUSPECT_CHANGES which might not cause *test* performance regressions.
Assignee | ||
Comment 49•14 years ago
|
||
I pushed with NS_MIN_INACTIVE_SUSPECT_CHANGES=5, but if it causes regressions, I'll backout.
Assignee | ||
Comment 50•14 years ago
|
||
Had to backout because of dromaeo dom regressions. But I have another idea, which I'll try tomorrow.
Assignee | ||
Comment 51•14 years ago
|
||
Pushed this to tryserver. (Well, I've pushed several different versions, but so far all of them have regressed dromaeo dom.)
Assignee | ||
Comment 52•14 years ago
|
||
Based on tryserver, this might not regress dromaeo DOM.
Attachment #486884 -
Flags: review?(jst)
Comment 53•14 years ago
|
||
Comment on attachment 486884 [details] [diff] [review] v7 - In js/src/xpconnect/src/nsXPConnect.cpp: +static PRUint32 gMainThreadEventCounter = 0; That looks like an unused global variable, remove it. r=jst with that.
Attachment #486884 -
Flags: review?(jst) → review+
Assignee | ||
Comment 54•14 years ago
|
||
Ah, a left-over from v5 or something.
Assignee | ||
Comment 55•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/592169fbecba Please reopen if you notice any perf test regression. So far I haven't seen any.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•