Closed Bug 1104151 Opened 10 years ago Closed 10 years ago

Attention indicator causes b2g main thread 10%-20% usage while in a Loop call.

Categories

(Firefox OS Graveyard :: Gaia::System, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.1+, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S8 (7Nov)
blocking-b2g 2.1+
Tracking Status
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: sotaro, Assigned: alive)

References

Details

+++ This bug was initially created as a clone of Bug #1097235 +++ From Bug 1097235 Comment 38, attention indicator consumes a lot of b2g main thread's cpu time. The attention indicator is implemented as css animation. From the analysis, the css animation are work as async animation. The async animation move the animation task to compositor side, therefore, b2g main thread task should be reduced to low cpu usage.
The following is HTML of Attention Indicator. > return '<div id="attention-indicator" class="attention-indicator" ' + > 'data-z-index-level="attention-indicator">' + > '<div class="handle handle1"></div>' + > '<div class="handle handle2"></div>' + > '<div class="handle handle3"></div>' + > '</div>'; https://git.mozilla.org/?p=releases/gaia.git;a=blob;f=apps/system/js/attention_indicator.js;h=cf1c45e97fc17cb205096f1927e7ed8db8bdf087;hb=refs/heads/v2.1#l32
When AsyncAnimationFailure log is enabled, logcat log did not have the animation error log.
b2g main thread was busy, because animation's throttling did not happen. AnimationPlayerCollection::CanThrottleAnimation() return false because FrameLayerBuilder::GetDedicatedLayer() return nullptr. http://dxr.mozilla.org/mozilla-central/source/layout/style/AnimationCommon.cpp#804 http://dxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#4138
In FrameLayerBuilder::GetDedicatedLayer(), the animated frame did not have LayerManagerDataProperty.
From FrameLayerBuilder::DisplayItemData::AddFrame(), all frames that are used in DisplayList should have LayerManagerDataProperty. Therefore, the animated frame seems not be used in DisplayList. http://dxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#75
In Attention Indicator, handle3 causes the problem. When I changed the handle3's attribute to "display: none" or "margin: -0.4333rem auto", the problem was fixed.
From comment 8, it seems like that the following attribute triggers the problem. > margin: -0.5333rem auto;
The handle3's computed attributes are following. They are got by using Firefox WebIDE. The elements height is 2.66667px, but margin-top is -5.33333px. Element seems to exist off screen. ------------------------------------ animation-delay 0.666s animation-direction normal animation-duration 1s animation-fill-mode none animation-iteration-count infinite animation-name handle animation-play-state running animation-timing-function cubic-bezier(0.25, 0.1, 0.25, 1) background-attachment scroll background-clip border-box background-color #B2F2FF background-image none background-origin padding-box background-position 0% 0% background-repeat repeat background-size auto auto color #FFF font-size 10px font-weight 500 height 2.66667px margin-bottom -5.33333px margin-left 0px margin-right 0px margin-top -5.33333px width 86px
DisplayItem is created by sIFrame::BuildDisplayListForChild(), the handle3's DisplayItem creation was failed in the following code, because, it does not have valid area. Both the dirty area and child frame area were (x,y,w,h)=(0,0,0,0). // We can stop if child's frame subtree's intersection with the // dirty area is empty. // If the child is a scrollframe that we want to ignore, then we need // to descend into it because its scrolled child may intersect the dirty // area even if the scrollframe itself doesn't. // There are cases where the "ignore scroll frame" on the builder is not set // correctly, and so we additionally want to catch cases where the child is // a root scrollframe and we are ignoring scrolling on the viewport. nsIPresShell* shell = PresContext()->PresShell(); bool keepDescending = child == aBuilder->GetIgnoreScrollFrame() || (shell->IgnoringViewportScrolling() && child == shell->GetRootScrollFrame()); if (!keepDescending) { nsRect childDirty; if (!childDirty.IntersectRect(dirty, child->GetVisualOverflowRect())) return; // Usually we could set dirty to childDirty now but there's no // benefit, and it can be confusing. It can especially confuse // situations where we're going to ignore a scrollframe's clipping; // we wouldn't want to clip the dirty area to the scrollframe's // bounds in that case. } } http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2323
From comment 11, Attention Indicator's handle3 is not rendered at all, just adds b2g main thread's cpu usage.
By the investigation, the following becomes clear. This is a gecko's defect. Talked with mstange and BenWa about this problem. It is not easy to fix on b2g v2.1. For b2g v2.1, it is better to fix the problem by changing gaia side. - When css animated element is positioned off screen, the element is not rendered at all, but it requests to render by 60fps.
The problem could be fixed by either ways. - [1] Remove Attention Indicator's handle3 - [2] Change Attention Indicator's handle3 position from offscreen to on screen.
Component: Graphics → Gaia::System
Product: Core → Firefox OS
[Blocking Requested - why for this release]: This bug increase b2g main thread's cpu usage. It could affect to heavy cpu usage applications like WebRTC.
blocking-b2g: --- → 2.1?
Changed the component to Gaia::System. For b2g v2.1, it is better to fix the problem by gaia side.
Bug 1104151 is created for gecko's side bug.
(In reply to Sotaro Ikeda [:sotaro] from comment #17) > Bug 1104151 is created for gecko's side bug. I guess you mean bug 1104726 is for the gecko side?
We need to figure out how much risk we are willing to take here before we can start working on it since it will be a 2.1 only patch.
Flags: needinfo?(fabrice)
Flags: needinfo?(bbajaj)
Alberto, please take a look at comment 14 and see if this is an easy fix for us.
Flags: needinfo?(apastor)
It seems that 'handle3' was removed in master: https://github.com/mozilla-b2g/gaia/commit/25452e1f5e22c7ab99d30817d842e66df5e435fc Can we try to uplift that? Alive, the bug number in the commit doesn't seem to match. Do you think is save to uplift that patch? Thanks!
Flags: needinfo?(apastor) → needinfo?(alive)
(In reply to Gregor Wagner [:gwagner] from comment #18) > (In reply to Sotaro Ikeda [:sotaro] from comment #17) > > Bug 1104151 is created for gecko's side bug. > > I guess you mean bug 1104726 is for the gecko side? Yes :-) sorry for my mistake.
(In reply to Alberto Pastor [:albertopq] from comment #21) > It seems that 'handle3' was removed in master: > > https://github.com/mozilla-b2g/gaia/commit/ > 25452e1f5e22c7ab99d30817d842e66df5e435fc > > Can we try to uplift that? Alive, the bug number in the commit doesn't seem > to match. Do you think is save to uplift that patch? Thanks! It is https://bugzilla.mozilla.org/show_bug.cgi?id=1072616 and fabrice deny my request :/ If the patch is helping with the performance here could you ask approval again?
Flags: needinfo?(alive)
:sotaro, could you please double check that the patch in Bug 1072616 fixes this performance issue? Thanks!
Flags: needinfo?(sotaro.ikeda.g)
I checked the latest master flame ROM. When Galley app is in foreground, b2g main thread seems busy because of attention notification animation. When enabling "Log Slow animations". Logcat has the following warning log. > Gecko : Performance warning: Async animation disabled because frame size (90, 4) is bigger than the viewport (360, 2) or the visual rectangle (104, 4) is larger than the max allowable value (17895698) [div] On master, attention notification is changed a lot, it seems to be affected to this.
Flags: needinfo?(sotaro.ikeda.g)
The log of Comment 25 is from nsDisplayTransform::ShouldPrerenderTransformedContent().
b2g main thread fps could be estimated from "Frames per second" that could be enabled from "Developer HUD". middle is "transaction fps" http://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/FPSCounter.cpp#445
(In reply to Alberto Pastor [:albertopq] from comment #24) > :sotaro, could you please double check that the patch in Bug 1072616 fixes > this performance issue? Thanks! On v2.1 flame, it works! But on master, async animation did not work as in comment 25. It seems to be caused by animated Element size problem. It is a different problem. It seems to be caused by re-factoring of attention indicator in master.
(In reply to Gregor Wagner [:gwagner] from comment #19) > We need to figure out how much risk we are willing to take here before we > can start working on it since it will be a 2.1 only patch. We have a low risk patch in https://bugzilla.mozilla.org/show_bug.cgi?id=1072616, that seems to help here.
blocking-b2g: 2.1? → 2.1+
Flags: needinfo?(bbajaj)
Flags: needinfo?(fabrice)
Can we verify that the patch from 1072616 fixed this problem and we can dupe?
Keywords: verifyme
Should be fixed by bug 1072616.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee: nobody → alive
Depends on: 1072616
Target Milestone: --- → 2.1 S8 (7Nov)
Clone bug 1127757 to track this bug.
Keywords: verifyme
Blocks: 1127757
You need to log in before you can comment on or make changes to this bug.