Closed Bug 508518 Opened 15 years ago Closed 14 years ago

Implement nsUITimerCallback with one-shot timer

Categories

(Core :: DOM: Core & HTML, defect)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: romaxa, Assigned: smaug)

References

Details

Attachments

(4 files, 5 obsolete files)

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.
Attachment #392687 - Flags: review?(Olli.Pettay)
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-
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 :/
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...
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...
(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())
(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.
>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
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?
Any code can add new cycle collectable objects. Whatever timers run and execute 
something, or callbacks which handle some network activity, or ...
(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?
Cycle collector has a list of suspected objects.
Romaxa, will you perhaps provide a new patch?
Blocks: 446418
(In reply to comment #13)
> Romaxa, will you perhaps provide a new patch?

I'll try.
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...
See how new objects are added to mPurpleBuf in cycle collector.
This is should be set to block bug 583135
 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?
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.
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?
tracking-fennec: --- → ?
Is the new cycle collector mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=564118#c17 going to have effect for this implementation?
(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.
(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.
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?
(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.
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)
tracking-fennec: ? → 2.0b2+
Assignee: nobody → azakai
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-
Attached patch Patch v2 (obsolete) — Splinter Review
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)
But cycle collector works only in main thread and cycle collectable
objects aren't thread safe.
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)
Attached patch Patch v3 (obsolete) — Splinter Review
Fixed patch. Everything now done correctly on the main thread.
Attachment #479931 - Flags: review?(bzbarsky)
Attachment #479931 - Flags: review?(Olli.Pettay)
So other than cycle collector, what else observes these notifications?  I assume you audited the consumers?
(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)?
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.
Attachment #479931 - Flags: review?(bzbarsky)
(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 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)
Attached patch add a missing null check (obsolete) — Splinter Review
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)
Attachment #480699 - Attachment is obsolete: true
Attachment #480758 - Flags: review?(jst)
Attachment #480699 - Flags: review?(jst)
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 on attachment 480758 [details] [diff] [review]
yet another null check for xpcshell tests

Looks good.
Attachment #480758 - Flags: review?(jst) → review+
Unfortunately this may caused dromaeo DOM regression.
I backed out.
I'll push to tryserver with a bit smaller NS_MIN_INACTIVE_SUSPECT_CHANGES.
That seems to compare dromaeo in the wrong way.
In dromaeo higher numbers are better, in many other tests
lower numbers are better.
> I'll push to tryserver with a bit smaller NS_MIN_INACTIVE_SUSPECT_CHANGES.

Did you manage to get rid of the perf regression?
No. Changing CC scheduling is tricky.
If we need to wait for this to be "right", we can push out of beta 2.
tracking-fennec: 2.0b2+ → 2.0b3+
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.
I pushed with NS_MIN_INACTIVE_SUSPECT_CHANGES=5, but if it causes regressions, 
I'll backout.
Had to backout because of dromaeo dom regressions.
But I have another idea, which I'll try tomorrow.
Attached patch v6Splinter Review
Pushed this to tryserver. (Well, I've pushed several different versions, but
so far all of them have regressed dromaeo dom.)
Attached patch v7Splinter Review
Based on tryserver, this might not regress dromaeo DOM.
Attachment #486884 - Flags: review?(jst)
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+
Ah, a left-over from v5 or something.
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.