Closed Bug 1023940 Opened 6 years ago Closed 6 years ago

Optimizations for gaia homescreen edit mode

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
2.1 S4 (12sep)

People

(Reporter: kgrandon, Assigned: BenWa)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 obsolete files)

This is a placeholder bug to track optimizing gfx code for the gaia homescreen edit mode.
Milan - Just doing a ni? on you here so you are aware. We are still in the process of making some final dragdrop improvements from the gaia side of things, and should have something landed this week - it's taken a bit longer than I would've hoped.

We don't plan to make any major changes though if possible, so you may be able to start doing some initial investigation here. I can help upload a profile this week if it would help.
Flags: needinfo?(milan)
Assignee: nobody → bgirard
Flags: needinfo?(milan)
Can you collect 2 profiles:
0) Make sure you built with 'export B2G_DUMP_PAINTING=1' in your userconfig.
1) First one using:
./profile.sh start -p b2g -t Compositor && ./profile.sh start -p Vertical
2) Second one (reboot in between):
set layout.display-list.dump;true via edit-prefs.sh
./profile.sh start -p b2g -t Compositor && ./profile.sh start -p Vertical
Kevin - Is this required to ship vertical homescreen? Or this is just a strong want, but we would ship without this if we ran out of time?
Flags: needinfo?(kgrandon)
QA Whiteboard: [VH-FL-blocking?][VH-FC-blocking?]
I would like to block for now, we really need this to be as polished as possible. Benoit - I'll work on getting you those profiles asap.
Flags: needinfo?(kgrandon)
Okay - Milan - Can you flag feature-b2g flag here, since we're claiming this is feature work & a blocker for ship in 2.0?
QA Whiteboard: [VH-FL-blocking?][VH-FC-blocking?] → [VH-FL-blocking+][VH-FC-blocking+]
Flags: needinfo?(milan)
Done - I like that better than making it 2.0+.  We're going to work on the 2.0 branch based solution, but will still ask for uplift approvals for individual patches.
feature-b2g: --- → 2.0
Flags: needinfo?(milan)
Benwa - I'm still unable to build for flames, but would profiles on a nexus4 help? If not I'll try to see if someone can try to get these for us.

First profile:
https://people.mozilla.org/~bgirard/cleopatra/#report=7e8b98504a78a8bd48b1db8cb89d210a6a0fa7ed

With: layout.display-list.dump = true
https://people.mozilla.org/~bgirard/cleopatra/#report=cfe7d3e398f8a859ef95985cb61d2dfbbaf1f83a

These are the profiles of a few dragdrop operations on the homescreen. I tried to use similar motions, but they probably aren't exact or anything. Let me know if these were helpful, if not I'll try again/on different hardware.
Flags: needinfo?(bgirard)
In the second profile you must of missed one of the step because the display list isn't in the profile. The profile also aren't synced up. We had that bug in 1.4 so I wonder if you perhaps have some resources that are out of date.

No worries, I'll have a look.
Flags: needinfo?(bgirard)
Depends on: 1026240
Whiteboard: [systemsfe]
Target Milestone: --- → 2.0 S4 (20june)
This drastically reduces the rasterization time and jank dragging the icon around.
Attachment #8442437 - Flags: review?(kgrandon)
Thanks. This is going to make the scrolling suffer again though, no? I'll run this by UX real quick, it seems like that may be ok in edit mode.
We shouldn't be scrolling much in edit mode. But yes it will make it suffer a bit until bug 1017251 is solved.

Actually I just realize you can be in edit mode and not be dragging any icon. So if you'd like we can activate will-change only when you're dragging an icon around. We really need will change when we're dragging icons because when we move the inactive icons around right now we have to do a lot of painting and we want that animation to be low latency, so IMO the right tool there is will-change: transform. Right now without it it's painfully slow.
After dogfooding for a while I noticed that scrolling in edit mode is rather common, you often want to rearrange icons from the top, to the bottom, so you make either minor or major scroll adjustments. The hit to scrolling is bad enough, that I don't think we should do it.

When you do begin a drag action though, there is already a delay for the context menu, so I think that might be a decent place to have will-change. There might be a bit of jankiness starting or stopping a drag though, if the user tries to scroll right after.
Attachment #8442501 - Flags: review?(bgirard)
Comment on attachment 8442437 [details] [diff] [review]
Part 1: Apply will-change only during edit-mode

Review of attachment 8442437 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing for now in case we decide to go with the version that has will-change for drag actions.
Attachment #8442437 - Flags: review?(kgrandon)
The hit to scrolling is not that noticeable on my end. Are you running with layers border enabled or some similar debugging feature?
(In reply to Benoit Girard (:BenWa) from comment #15)
> The hit to scrolling is not that noticeable on my end. Are you running with
> layers border enabled or some similar debugging feature?

It's similar when I opened the other bug, and you suggested that we remove will-change. (I can find that again and show you to video if you want). It's mostly noticeable on APZ deceleration. 

I don't have any debugging features turned on.
Yes, but I had layers border on when I saw it which multiplies the hit. It's best to not count on us speed up that code path in time.

Alright let's only apply will-change when you're actively dragging icons then.
Attachment #8442437 - Attachment is obsolete: true
Comment on attachment 8442501 [details] [review]
Part 1 - Apply will-change when dragging an icon

A review from either of you guys would be fine for this. Thanks.
Attachment #8442501 - Attachment description: Part - Apply will-change when dragging an icon → Part 1 - Apply will-change when dragging an icon
Attachment #8442501 - Flags: review?(crdlc)
Comment on attachment 8442501 [details] [review]
Part 1 - Apply will-change when dragging an icon

LGTM
Attachment #8442501 - Flags: review?(crdlc) → review+
Attachment #8442501 - Flags: review?(bgirard)
As discussed in our standup we landed will-change support as the first part of this bug. This is good enough for FL quality, and additional work will be uplifted.

Candice - can you remove the FL and FC blocking status? Thanks!
Flags: needinfo?(cserran)
QA Whiteboard: [VH-FL-blocking+][VH-FC-blocking+] → [VH-FL-blocking-][VH-FC-blocking-]
feature-b2g: 2.0 → ---
Flags: needinfo?(cserran)
Attachment #8442501 - Attachment is obsolete: true
Depends on: 1028568
Blocks: vertical-home-next
No longer blocks: vertical-homescreen
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Depends on: 1034347
Target Milestone: 2.0 S5 (4july) → 2.0 S6 (18july)
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
I've got patches for bug 1010584 that are nearly ready. I expect that this will be the last piece to resolve this issue.
Depends on: 1010584
Target Milestone: 2.1 S1 (1aug) → 2.1 S2 (15aug)
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Is this still in progress? There hasn't been any movement on this for a couple sprints now. Please update target milestone or close. Thanks
Flags: needinfo?(kgrandon)
Dependent bugs are closed, so I think we can call this one fixed now.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(kgrandon)
Resolution: --- → FIXED
Target Milestone: 2.1 S3 (29aug) → 2.1 S4 (12sep)
You need to log in before you can comment on or make changes to this bug.