[System] Device does not enter cpu idle state (AP's core 0 voltage)

RESOLVED FIXED in Firefox 25, Firefox OS v1.1hd

Status

Firefox OS
General
P1
major
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Leo, Assigned: nrc)

Tracking

unspecified
1.1 QE3 (26jun)
x86
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:leo+, firefox23 wontfix, firefox24 wontfix, firefox25 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: TaipeiWW)

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

5 years ago
+++ This is a followup of  Bug #877215  +++

1. Title : Device does not enter cpu idle state (AP's core 0 voltage)
2. Precondition : Turn on deivce
3. Tester's Action : Check device state after turn on.
4. Detailed Symptom :  When device is turned on, the AP's core voltage never goes down.
5. Expected : Device should enter cpu idle state (AP's core 0 voltage), when there is no input for certain period of time.
6. Reproducibility: Y
1) Frequency Rate : 100%
7. Gaia revision : 

As per the comment 12 of Bug #877215 filing a follow up bug.
(Reporter)

Updated

5 years ago
Severity: normal → major
Priority: -- → P1
ni? alive/yuren as per https://bugzilla.mozilla.org/show_bug.cgi?id=877215#c14
would be great if you guys can look at this.
thanks!
blocking-b2g: --- → leo?
Flags: needinfo?(yurenju.mozilla)
Flags: needinfo?(alive)
See Also: → bug 877215
Revert this patch still got some process hold the "unknown_suspend" wakelock
"Bug 877214 - Toggle lockscreen animation according to visibilitychange"
Assignee: nobody → rlin

Comment 3

5 years ago
Revert? What do you mean?

Comment 5

5 years ago
Why did you roll back the change? Is it wrong?
Hi Andreas,
It just for my local test, not blackout it from repository.

Comment 7

5 years ago
Why did you back it out locally?
Hi Andreas,
I'm curious about Leo say this patch cause this problem:
https://bugzilla.mozilla.org/show_bug.cgi?id=877215#c13
 Leo 2013-05-31 03:10:29 PDT
When I role back patch of Bug 838486(apply master git on 3/13), tester said device can enter CPU Idle state.
----
I will try to compare the idle current between Leo and Unagi.

Comment 9

5 years ago
Hi all,

I think wakelock is only relevant to "device suspend state" which is made by pressing power key or timeout of idle timer from system app.

But the bug here mentioned to "CPU Idle State" which is no relationship with wakelock. It is depend on the timeout value of nearest timer. When kernel's idle thread (lowest priority) got the chance to run it will detect the value of next timer then decide the power saving level (spin, SWFI, Standalone power collapse). In this case, it may caused by some Gaia/Gecko components fired a shorter timer periodically.

Hi Leo,

Could you check the reason for cpu can't go into idle state in kernel's point of view?
If shorter time is the reason then we can help to check on Gaia/Gecko side.
Flags: needinfo?(leo.bugzilla.gecko)
I'll help Randy to investigate this issue.
Flags: needinfo?(yurenju.mozilla)

Comment 11

5 years ago
Thanks Marco! Thats what I suspected. Yuren, you guys should run the gecko process in GDB and break on the main event loop and see how often we wake up while sleeping and then figure out why.
Update to 0602 mozilla-b2g18-leo-eng build, turn the back light to minimal, the current is 45mA. After stop b2g the current is down to ~43ma.
check the top  -m 10 -t -d 10
  865   865  0   1% S 199756K  61620K  fg root     b2g             /system/b2g/b2g
  161   212  0   1% S   5908K    468K  fg root     sensord         /system/bin/sensord
  865   882  0   1% S 199756K  61620K  fg root     Timer           /system/b2g/b2g
 1097  1097  0   0% R   1072K    464K  fg root     top             top
Created attachment 757425 [details]
profile for idle

Profile the b2g process, found nsRefreshDriver::Notify.

Comment 14

5 years ago
Can you post a stack?
also found 
CC::nsCycleCollector_collect, GC::GarbageCollectNow hit many times on Timer.
logging at nsTimerImpl.cpp:475, 
mClosure, callback.c, callbackType, time
0x0 cb = 0x48101de0 type = 1 , time:19:58:07 294705 
0x0 cb = 0x48101de0 type = 1 , time:19:58:07 311886 
0x0 cb = 0x48101de0 type = 1 , time:19:58:07 329220 
0x0 cb = 0x48101de0 type = 1 , time:19:58:07 346677 
0x0 cb = 0x48101de0 type = 1 , time:19:58:07 363553 
0x0 cb = 0x48101de0 type = 1 , time:19:58:07 380765 
0x0 cb = 0x48101de0 type = 1 , time:19:58:07 397977 
0x0 cb = 0x48101de0 type = 1 , time:19:58:07 415097 
0x0 cb = 0x48101de0 type = 1 , time:19:58:07 432187 
0x0 cb = 0x48101de0 type = 1 , time:19:58:07 449460 
0x0 cb = 0x48101de0 type = 1 , time:19:58:07 466611 
0x0 cb = 0x48101de0 type = 1 , time:19:58:07 483761 
(gdb) p callback.c
$1 = (nsTimerCallbackFunc) 0x48101de0
Is this normal?
keep nsRefreshDriver::Notify(nsITimer *aTimer)...
notify on nsRefreshDriver
3858 0x0 cb = 0x450e80c0 type = 1 , time:23:59:50 899014 
notify on nsRefreshDriver
3985 0x0 cb = 0x42becf50 type = 1 , time:23:59:50 902798 
notify on nsRefreshDriver
3858 0x0 cb = 0x450e80c0 type = 1 , time:23:59:50 916562 
notify on nsRefreshDriver
3985 0x0 cb = 0x42becf50 type = 1 , time:23:59:50 920590 
notify on nsRefreshDriver
3858 0x0 cb = 0x450e80c0 type = 1 , time:23:59:50 934354 
notify on nsRefreshDriver
3985 0x0 cb = 0x42becf50 type = 1 , time:23:59:50 938626 
notify on nsRefreshDriver
3858 0x0 cb = 0x450e80c0 type = 1 , time:23:59:50 950925 

But I found sometimes I can't found this symption by disable home screen directly, but keep refresh when enter setting apps.

Updated

5 years ago
Flags: needinfo?(leo.bugzilla.gecko)
Leo has provided information to identify the CPU has entered idle mode or not.
We can cat /proc/msm_pm_stats and check idle-power-collapse item to see if count has increased or not. But this method isn't accurate. The better way is checking the clk of cpu and check the wave form.
Flags: needinfo?(alive)
It looks like the animation of lock-screen is not ended after unlocked.
Just remove the animation works, but reset animation to none doesn't work. Strange.
Created attachment 759682 [details] [diff] [review]
check the reFreshDriver is idle or not

Can apply this patch to check if the nsRefreshDriver timer is stoped or not.
Directly run b2g.sh and check the console's output log timimg is keep changing.
7302 0x0 cb = 0x47e623d0 type = 1 , time:6:27:26 893244

Comment 21

5 years ago
Randy try to figure out why the refresh driver doesn't stop.
OK, I am digging out the root-casue.
Found the timer is failed to be removed, a little strange.
made a webpage with the same animation as lockscreen.
http://yurenju.github.io/animationtest/
In nsRefreshDriver.cpp, When this symption happen, the mObservers[0].Length() keeps > 0 (Flush_Style) , and ObserverCount() keeps return 1, so the StopTimer() won't be called, should found which one miss call the RemoveRefreshObserver().
blocking-b2g: leo? → leo+
(Assignee)

Comment 25

5 years ago
(In reply to Randy Lin [:rlin] from comment #24)
> In nsRefreshDriver.cpp, When this symption happen, the
> mObservers[0].Length() keeps > 0 (Flush_Style) , and ObserverCount() keeps
> return 1, so the StopTimer() won't be called, should found which one miss
> call the RemoveRefreshObserver().

Did you manage to reproduce this behaviour on desktop or only on a phone?

I am looking at desktop (with OMTC and OMTA to be a bit closer to b2g-land) and I do not observe this - I see StopTimer getting called from nsRefreshDriver::Tick, but in RemoveRefreshDriver we sometimes don't stop the timer because there are other refresh drivers still on the timer (in fact we seem to get 'locked into' this situation and never get down to zero refresh drivers, although not every run).

On the phone does the refresh driver always keep going or only sometimes?
Flags: needinfo?(rlin)
Hi Nick,
We found this issue at b2g18 branch on b2g platform.
You can check the refresh driver behavior after unlocking the cell phone.
But if I disabled the lock screen through "setting->phone lock->disable" then re-boot the device, this symptom was gone.
Flags: needinfo?(rlin)
(Reporter)

Updated

5 years ago
Target Milestone: --- → 1.1 QE3 (24jun)
(Assignee)

Comment 27

5 years ago
Using an up to date Gecko, this does not seem to be a problem, or at least I could not repro. Trying with b2g18 now...
Target Milestone: 1.1 QE3 (24jun) → ---
(Assignee)

Comment 28

5 years ago
Hmm, so far I have not managed to observe this behaviour - the timer on the refresh driver is being stopped every now and again. So StopTimer does get called. The only odd thing I noticed is that RmoveRefreshObserver gets called twice for each observer, but this doesn't seem to harm anything.
Target Milestone: --- → 1.1 QE3 (24jun)
(Assignee)

Comment 29

5 years ago
After a few more runs I've noticed some pretty long periods of time where the refresh driver keeps going, but it always eventually stops. I kind of assume it is meant to be on - perhaps it is when I've left the lock screen going.

Perhaps I'm not clear on the STR - I am unlocking the device, then using some things from the home screen and noting if the refresh driver is always on. Also tried unlocking and then doing nothing. I have also waited for the animation to play and then unlock.

Are we instead talking about not unlocking and letting the screen turn off? Or not unlocking whilst the screen remains on?
(Assignee)

Updated

5 years ago
Flags: needinfo?(rlin)
Hi Nick,
We found there are some code follow invoke the AddElementData() in nsAnimationManager, but miss the RemoveAllElementData(), is it normal?
The STR is 1. boot-up device, 2. unlock lockscreen, enter home screen 3. keep the screen always on.
Flags: needinfo?(rlin)
(Assignee)

Comment 31

5 years ago
(In reply to Randy Lin [:rlin] from comment #30)
> Hi Nick,
> We found there are some code follow invoke the AddElementData() in
> nsAnimationManager, but miss the RemoveAllElementData(), is it normal?

I think it could be normal, the transition or animation managers could remove a single element data manually, as long as they call ElementDataRemoved afterwards then it is OK. If they don't call that, then we could get this kind of error.

> The STR is 1. boot-up device, 2. unlock lockscreen, enter home screen 3.
> keep the screen always on.

Thanks! I think I covered doing this and couldn't repro :-( Exactly which b2g gecko repo are you using (I pulled b2g18 with no suffix).
The symptom is also seen on m-c. What is seen is that ElementAnimation instances are created but Destroy() is never called for them. nsAnimationManager only Destroy() an ElementAnimation instance in nsAnimationManager::CheckAnimationRule() but from the tests we never reach there.
Target Milestone: 1.1 QE3 (24jun) → ---

Updated

5 years ago
Whiteboard: TaipeiWW

Comment 33

5 years ago
jet can we get some help here? this is probably dbaron or bz territory
(Assignee)

Comment 34

5 years ago
Created attachment 765711 [details] [diff] [review]
possible fix by not adding empty animations in BuildAnimations

This patch causes the element data to actually get removed once the animation ends. I'm not 100% sure it's the right approach, but lets see if it works first. It works for me on desktop, but I couldn't repro the power issue. Could one of you see if this fixes the issue on a device please?
Attachment #765711 - Flags: feedback?(rlin)
Attachment #765711 - Flags: feedback?(cyu)
Hi Nick,
By observing the invoke of the nsRefreshDriver::Tick function call,
apply your patch and seems helpful on mozilla_central branch.
Comment on attachment 765711 [details] [diff] [review]
possible fix by not adding empty animations in BuildAnimations

Extra refresh driver tick is not seen after applying this patch.
Attachment #765711 - Flags: feedback?(cyu) → feedback+
Also test on b2g-18 branch, it works. No extra invoke of the nsRefreshDriver::Notify function.
Check the idle-power-collapse in /proc/msm_pm_stats, 
The count increases
idle-power-collapse:
  count:      67
  total_time: 2.523467881
===================================================
idle-power-collapse:
  count:    2267
  total_time: 129.211636297
(Assignee)

Updated

5 years ago
Attachment #765711 - Flags: review?(dbaron)
Comment on attachment 765711 [details] [diff] [review]
possible fix by not adding empty animations in BuildAnimations

This changes the behavior when there are dynamic changes to animation rules.  We currently (per spec, I believe) preserve the start time and delay state of an animation as long as the animation is in the list of animations; this adds some complex conditions to that that I wouldn't want to expose to the Web.

Instead, I think CommonElementAnimationData should have a virtual method for whether that AnimationData/TransitionData requires refreshes:  transitions should return true, and animations should return its mNeedsRefreshes.  Then CommonAnimationManager::AddElementData and CommonAnimationManager::ElementDataRemoved should call a common method that uses said method (plus a boolean to track whether the manager is currently a refresh observer), and nsAnimationManager should call that method as well when it might have changed mNeedsRefreshes.  Or something like that.

(Yeah, this was something I meant to do after implementing animations but never got back to doing.  Bug 649238.  Yikes.)
Attachment #765711 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #39)
> Instead, I think CommonElementAnimationData should have a virtual method for
> whether that AnimationData/TransitionData requires refreshes:  transitions
> should return true, and animations should return its mNeedsRefreshes.  Then
> CommonAnimationManager::AddElementData and
> CommonAnimationManager::ElementDataRemoved should call a common method that
> uses said method (plus a boolean to track whether the manager is currently a
> refresh observer), and nsAnimationManager should call that method as well
> when it might have changed mNeedsRefreshes.  Or something like that.

Or maybe, instead of virtual methods, nsTransitionManager should keep the current code (moving from CommonAnimationManager) and nsAnimationManager should just have separate code like the above.
(Assignee)

Comment 41

5 years ago
Created attachment 765858 [details] [diff] [review]
be more precise about removing the animation manager from the refresh driver

dbaron: Is this what you mean? I think I followed your suggestion, but I wasn't 100% clear. We seem to call CheckNeedsRefresh() a lot, but I couldn't find a way to reduce the number of calls. Not sure if it will be a performance issue, hopefully it's not too bad.

rlin: I see this reducing load on the refresh driver in the same was as the last patch on desktop, but if you could double check on a device, that would be great!
Attachment #765711 - Attachment is obsolete: true
Attachment #765858 - Flags: review?(dbaron)
Attachment #765858 - Flags: feedback?(rlin)
(Assignee)

Comment 42

5 years ago
Created attachment 765869 [details] [diff] [review]
be more precise about removing the animation manager from the refresh driver

and a version which actually builds (thanks to rlin for finding the bug, and verifying the previous version worked).
Attachment #765858 - Attachment is obsolete: true
Attachment #765858 - Flags: review?(dbaron)
Attachment #765858 - Flags: feedback?(rlin)
Attachment #765869 - Flags: review?(dbaron)
Comment on attachment 765869 [details] [diff] [review]
be more precise about removing the animation manager from the refresh driver

ElementDataRemoved and AddElementData should be both
MOZ_OVERRIDE *and* MOZ_FINAL on the transition manager and
animation manager... at least assuming you can do that.

I think the removal of the ElementDataRemoved call from
FlushTransitions was a mistake -- that breaks exactly this
in the transition manager.


I don't see why you're adding a distinction between 
GetPositionInIteration and GetPositionInIterationInternal when you're
not changing anything in either function except for the fact that
GetPositionInIteration doesn't pass through the last 3 parameters to
GetPositionInIterationInternal.  You should undo that.


I need to think a little more about whether the CheckNeedsRefresh calls
are in sufficient places.
Attachment #765869 - Flags: review?(dbaron) → review-
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #43)
> Comment on attachment 765869 [details] [diff] [review]
> be more precise about removing the animation manager from the refresh driver
> 
> ElementDataRemoved and AddElementData should be both
> MOZ_OVERRIDE *and* MOZ_FINAL on the transition manager and
> animation manager... at least assuming you can do that.

Er, I guess it's the clases nsAnimationManager and nsTransitionManager that should be marked as MOZ_FINAL.  (The reason I care is that hopefully that will allow the compiler to get rid of the virtual function call overhead when the call is from within the class rather than from the base class.)
(Assignee)

Comment 45

5 years ago
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #43)
> Comment on attachment 765869 [details] [diff] [review]
> be more precise about removing the animation manager from the refresh driver
> 
> ElementDataRemoved and AddElementData should be both
> MOZ_OVERRIDE *and* MOZ_FINAL on the transition manager and
> animation manager... at least assuming you can do that.
> 

OK done, on the class.

> I think the removal of the ElementDataRemoved call from
> FlushTransitions was a mistake -- that breaks exactly this
> in the transition manager.
> 

I think this is ok because ElementDataRemoved gets called from the destructor of CommonElementAnimationData. I think I observe this behaviour. Am I missing something?

> 
> I don't see why you're adding a distinction between 
> GetPositionInIteration and GetPositionInIterationInternal when you're
> not changing anything in either function except for the fact that
> GetPositionInIteration doesn't pass through the last 3 parameters to
> GetPositionInIterationInternal.  You should undo that.
> 
The intent here is so I can have a public and private version of GetPosition... Callers of the internal version must always call CheckNeedsRefresh afterwards. Callers of the external version don't. I thought it was risky asking clients to call CheckNeedsRefresh (and in the existing case the caller can't because they don't have an animation manager). I thought this approach was clearer than saying something like "call CheckNeedsRefresh after this if aEa and aAnimation are non-null" in a comment - that felt fragile. If you don't think it is worse, then I can do it that way. I should make the comments clearer in this case if you think it is OK to leave it as is.

> 
> I need to think a little more about whether the CheckNeedsRefresh calls
> are in sufficient places.

There should be calls after every possible modification to mNeedsRefreshes, but sometimes moved up the call stack to somewhere that I have a reference to an animation manager and/or to coalesce the calls a bit.
(In reply to Nick Cameron [:nrc] from comment #45)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #43)
> > Comment on attachment 765869 [details] [diff] [review]
> > be more precise about removing the animation manager from the refresh driver

> > I think the removal of the ElementDataRemoved call from
> > FlushTransitions was a mistake -- that breaks exactly this
> > in the transition manager.
> > 
> 
> I think this is ok because ElementDataRemoved gets called from the
> destructor of CommonElementAnimationData. I think I observe this behaviour.
> Am I missing something?

Ah, indeed.  I missed that.

> > I don't see why you're adding a distinction between 
> > GetPositionInIteration and GetPositionInIterationInternal when you're
> > not changing anything in either function except for the fact that
> > GetPositionInIteration doesn't pass through the last 3 parameters to
> > GetPositionInIterationInternal.  You should undo that.
> > 
> The intent here is so I can have a public and private version of
> GetPosition... Callers of the internal version must always call
> CheckNeedsRefresh afterwards. Callers of the external version don't. I
> thought it was risky asking clients to call CheckNeedsRefresh (and in the
> existing case the caller can't because they don't have an animation
> manager). I thought this approach was clearer than saying something like
> "call CheckNeedsRefresh after this if aEa and aAnimation are non-null" in a
> comment - that felt fragile. If you don't think it is worse, then I can do
> it that way. I should make the comments clearer in this case if you think it
> is OK to leave it as is.

I need to look into this more, but I'm still skeptical.

> > I need to think a little more about whether the CheckNeedsRefresh calls
> > are in sufficient places.
> 
> There should be calls after every possible modification to mNeedsRefreshes,
> but sometimes moved up the call stack to somewhere that I have a reference
> to an animation manager and/or to coalesce the calls a bit.

That sounds sufficient (given the calls to ElementDataRemoved that should cover the removal cases).
(Reporter)

Updated

5 years ago
Target Milestone: --- → 1.1 QE3 (24jun)
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #46)
> (In reply to Nick Cameron [:nrc] from comment #45)
> > (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> > comment #43)
> > > Comment on attachment 765869 [details] [diff] [review]
> > > I don't see why you're adding a distinction between 
> > > GetPositionInIteration and GetPositionInIterationInternal when you're
> > > not changing anything in either function except for the fact that
> > > GetPositionInIteration doesn't pass through the last 3 parameters to
> > > GetPositionInIterationInternal.  You should undo that.
> > > 
> > The intent here is so I can have a public and private version of
> > GetPosition... Callers of the internal version must always call
> > CheckNeedsRefresh afterwards. Callers of the external version don't. I
> > thought it was risky asking clients to call CheckNeedsRefresh (and in the
> > existing case the caller can't because they don't have an animation
> > manager). I thought this approach was clearer than saying something like
> > "call CheckNeedsRefresh after this if aEa and aAnimation are non-null" in a
> > comment - that felt fragile. If you don't think it is worse, then I can do
> > it that way. I should make the comments clearer in this case if you think it
> > is OK to leave it as is.
> 
> I need to look into this more, but I'm still skeptical.

I still think it's unnecessary complexity -- I think you should just document that callers that pass non-null values for the last 3 parameters (that is, all callers except OMTA) are responsible for calling CheckNeedsRefreshes.
(BTW, I expect to mark review+ on a patch with those issues fixed, in case that wasn't clear.)
(Assignee)

Comment 49

5 years ago
Created attachment 767037 [details] [diff] [review]
patch with review comments addressed
Attachment #765869 - Attachment is obsolete: true
Attachment #767037 - Flags: review?(dbaron)
Comment on attachment 767037 [details] [diff] [review]
patch with review comments addressed

>diff --git a/layout/style/AnimationCommon.h b/layout/style/AnimationCommon.h

>-  ~CommonElementAnimationData()
>+  virtual ~CommonElementAnimationData()
>   {
>     NS_ABORT_IF_FALSE(mCalledPropertyDtor,
>                       "must call destructor through element property dtor");
>     MOZ_COUNT_DTOR(CommonElementAnimationData);
>     PR_REMOVE_LINK(this);
>     mManager->ElementDataRemoved();
>   }

I'd prefer not to do this; it's unnecessary virtual function call overhead.

The derived classes of CommonElementAnimationData are deleted only through the property destructors (as as this code asserts).  I'm guessing you did this to fix a compiler warning, but I think the right way to fix such a warning is something more like:
 * make ElementTransitions and ElementAnimations MOZ_FINAL (**already done**)
 * make ~CommonElementAnimationData protected
 * if needed, add explicit empty destructors for ElementTransitions and ElementAnimations, marked public (or, alternately, make ElementTransitionsPropertyDtor and ElementAnimationsPropertyDtor friends)

For now, could you at least undo this change?


>diff --git a/layout/style/nsAnimationManager.h b/layout/style/nsAnimationManager.h
>@@ -139,16 +139,18 @@ struct ElementAnimations MOZ_FINAL
>   // that if the animation has not started yet, has already ended, or is paused,
>   // it should not be run from the compositor.  When this function is called 
>   // from the main thread, we need the actual ElementAnimation* in order to 
>   // get correct animation-fill behavior and to fire animation events.
>   // This function returns -1 for the position if the animation should not be
>   // run (because it is not currently active and has no fill behavior), but
>   // only does so if aAnimation is non-null; with a null aAnimation it is an
>   // error to give aCurrentTime < aStartTime, and fill-forwards is assumed.
>+  // GetPositionInIteration with non-null aAnimation and aEa be sure to call
>+  // CheckNeedsRefresh on the animation manager afterwards.
>   static double GetPositionInIteration(TimeDuration aElapsedDuration,
>                                        TimeDuration aIterationDuration,
>                                        double aIterationCount,
>                                        uint32_t aDirection,
>                                        ElementAnimation* aAnimation = nullptr,
>                                        ElementAnimations* aEa = nullptr,
>                                        EventArray* aEventsToDispatch = nullptr);
> 

How about putting "After calling" at the beginning of your addition, and a comma before the "be sure to"?

  


r=dbaron with that
Attachment #767037 - Flags: review?(dbaron) → review+
(Assignee)

Comment 51

5 years ago
Created attachment 767085 [details] [diff] [review]
patch with changes

carrying r=dbaron
Attachment #767037 - Attachment is obsolete: true
Attachment #767085 - Flags: review+
(Assignee)

Comment 52

5 years ago
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #50)
> Comment on attachment 767037 [details] [diff] [review]
> patch with review comments addressed
> 
> >diff --git a/layout/style/AnimationCommon.h b/layout/style/AnimationCommon.h
> 
> >-  ~CommonElementAnimationData()
> >+  virtual ~CommonElementAnimationData()
> >   {
> >     NS_ABORT_IF_FALSE(mCalledPropertyDtor,
> >                       "must call destructor through element property dtor");
> >     MOZ_COUNT_DTOR(CommonElementAnimationData);
> >     PR_REMOVE_LINK(this);
> >     mManager->ElementDataRemoved();
> >   }
> 
> I'd prefer not to do this; it's unnecessary virtual function call overhead.
> 
> The derived classes of CommonElementAnimationData are deleted only through
> the property destructors (as as this code asserts).  I'm guessing you did
> this to fix a compiler warning, but I think the right way to fix such a
> warning is something more like:
>  * make ElementTransitions and ElementAnimations MOZ_FINAL (**already done**)
>  * make ~CommonElementAnimationData protected
>  * if needed, add explicit empty destructors for ElementTransitions and
> ElementAnimations, marked public (or, alternately, make
> ElementTransitionsPropertyDtor and ElementAnimationsPropertyDtor friends)
> 
> For now, could you at least undo this change?
> 

Sure. There was no compiler warning, it was one of my first guesses for what was going wrong and the missing virtual made me a bit twitchy.

> 
> >diff --git a/layout/style/nsAnimationManager.h b/layout/style/nsAnimationManager.h
> >@@ -139,16 +139,18 @@ struct ElementAnimations MOZ_FINAL
> >   // that if the animation has not started yet, has already ended, or is paused,
> >   // it should not be run from the compositor.  When this function is called 
> >   // from the main thread, we need the actual ElementAnimation* in order to 
> >   // get correct animation-fill behavior and to fire animation events.
> >   // This function returns -1 for the position if the animation should not be
> >   // run (because it is not currently active and has no fill behavior), but
> >   // only does so if aAnimation is non-null; with a null aAnimation it is an
> >   // error to give aCurrentTime < aStartTime, and fill-forwards is assumed.
> >+  // GetPositionInIteration with non-null aAnimation and aEa be sure to call
> >+  // CheckNeedsRefresh on the animation manager afterwards.
> >   static double GetPositionInIteration(TimeDuration aElapsedDuration,
> >                                        TimeDuration aIterationDuration,
> >                                        double aIterationCount,
> >                                        uint32_t aDirection,
> >                                        ElementAnimation* aAnimation = nullptr,
> >                                        ElementAnimations* aEa = nullptr,
> >                                        EventArray* aEventsToDispatch = nullptr);
> > 
> 
> How about putting "After calling" at the beginning of your addition, and a
> comma before the "be sure to"?
> 
>

Yes.

> 
> 
> r=dbaron with that
(Assignee)

Comment 54

5 years ago
hmm, which b2g tree should I land this too? And do I need approval?
Flags: needinfo?(dscravaglieri)

Comment 55

5 years ago
You want to land this on 1.1, and 1.1HD. Its a blocker so no approval needed. Great work by the way. ++nrc
Flags: needinfo?(dscravaglieri)
(In reply to Nick Cameron [:nrc] from comment #53)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/28251f4140f6

This change seems to improve sunspider & kraken performances on awfy (B2G's browser).
I am not sure how this is related to this patch, is that expected?

You can find a copy of sunspider[1] and kraken[2] benchmarks in my people account.

[1] http://people.mozilla.com/~npierron/sunspider/hosted/ (click on start now)
[2] http://people.mozilla.com/~npierron/kraken/hosted/ (click on begin)
https://hg.mozilla.org/mozilla-central/rev/28251f4140f6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 60

5 years ago
Created attachment 767558 [details] [diff] [review]
patch rebased for b2g18
(Assignee)

Comment 61

5 years ago
b2g18: https://hg.mozilla.org/releases/mozilla-b2g18/rev/fc88252e24e2
status-b2g18: --- → fixed
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/fc88252e24e2
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → fixed
status-firefox23: --- → wontfix
status-firefox24: --- → wontfix
status-firefox25: --- → fixed
(Assignee)

Updated

4 years ago
Depends on: 894497
Component: Gaia::System → General
You need to log in before you can comment on or make changes to this bug.