Closed Bug 967884 Opened 11 years ago Closed 6 years ago

[Contacts] visibility_monitor.js script taking 200ms+ and likely causing reflows

Categories

(Firefox OS Graveyard :: Gaia::Contacts, defect, P3)

Product:
Firefox OS Graveyard
Component:
Gaia::Contacts
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: BenWa, Unassigned)

References

Details

(Keywords: perf, Whiteboard: [c=handeye p=5 s= u=])

Attachments

(3 obsolete files) 

Here's a profile:
http://people.mozilla.org/~bgirard/cleopatra/#report=feacfdfc1f931f13f5cc171f4a5ee219d392086e

I think visibility_monitor is trying to minimize the work done by layout. It was done in bug 865750. What causes us to pursue this solution?

Was the platform/layout team consulting in coming up with this solution. The reason I'm asking if because doing this kind of change actually breaks a lot of the platform optimizations as we can see in this profile so we need to be careful with an optimization like this.
Blocking a blocker
blocking-b2g: --- → 1.3+
It seems we are getting scroll events less frequently which is causing work to back up in the visibility monitor.

Here is a profile from bug 937425 on v1.2:

  http://people.mozilla.org/~bgirard/cleopatra/#report=dbd2827c8fcc687cc54223cc915007af7a2ed476&search=scrollHandler

There the visibility monitor fires roughly every 100ms.  In the profile in comment 0 it appears we only get the scroll event once every 250ms.

I'm not sure anyone loves the visibility monitor approach for contacts right now, but there are competing requirements in place:

1) Low time to above-the-fold display.
2) Low time to allow user to jump to end of list by pressing Z in the jump list.
3) High frames-per-second during scrolling.

In an ideal world we would have access to the contacts data via a cursor which supported all our data normalization and app-specific sorting.  This would allow us to build a small viewable range of DOM that could be quickly cycled and jumped around the list.

Unfortunately, though, the current contacts API essentially forces us to load the entire list in a single API call.  It has cursors, but we can't use them because the cursors don't support our sorting or name normalization rules.

The visibility monitor was an attempt to lazy load as much data as we could in order to meet goal (2) above.  Again, a rewrite to use a DOM cycling approach would probably be better, even without a cursor source.  Unfortunately, though, that is probably not a quick or trivial change.

Kevin, do you have anything to add here?  Have I mispoken about any of the constrains?
Flags: needinfo?(kgrandon)
If we can't increase the frequency of scroll events, then we might be able to address the problem by tweaking the visibility monitor parameters and modifying the list onscreen() callback to chunk code.  It would also be nice to try changing things to use requestAnimationFrame() instead of the timeouts we're currently using.
And of course, any suggestions on how to improve the contacts list code are greatly appreciated.
I had a conversation with Ehsan who convinced me this was a reasonable thing to do given the limitation of the web platform. I'll have a look tomorrow and see if I can give any suggestions on how we can speed up this script.
(In reply to Ben Kelly [:bkelly] from comment #2)
> There the visibility monitor fires roughly every 100ms.  In the profile in
> comment 0 it appears we only get the scroll event once every 250ms.

Assuming this is the "scroll" event you're talking about, the rate at which it is dispatched is the higher of the apz.pan_repaint_interval and how long it takes to do a paint. apz.pan_repaint_interval is currently defaulting to 250ms so that's likely why you're seeing that as the rate. When we're in a fling though (finger has left the screen) we use a repaint interval of 75ms. I don't know offhand why those two are so far apart, but perhaps you can confirm that the scroll events arrive every 75ms during the fling portion. If so it might be worth doing more frequent repaints - 250ms does seem kind of high to me considering we're throttling it based on paint time anyway.
Keywords: perf
Priority: -- → P1
Whiteboard: [c=handeye p= s= u=1.3]
Hi, I actually had a chat with Vivien about this issue this week. With two patches (one being the position sticky in bug 942460), we were able to remove all reflows from the contacts scrolling. Performance is better, but I'm not sure if we can fix all of the checkboarding with just these patches, though I will investigate. I was told that we were considering uplifting all sticky patches to 1.3, so I will try to get any remaining reviews I have for this patch.

In 1.5 or 1.6 I want to revisit the infinite scrolling list ideas I have. We are also waiting on some platform support which will provide events for when we reach the end of a scrollable area. This should help us in creating truly streaming lists.

As this is now 1.3+, I'll assign this to myself, and work with Vivien today to get a patch here. Vivien - NI? you so you are aware. I'll sit with you to get that patch done this week, or you can steal this from me if you'd like.
Assignee: nobody → kgrandon
Flags: needinfo?(kgrandon) → needinfo?(21)
Whiteboard: [c=handeye p= s= u=1.3] → [c=handeye p=3 s= u=1.3]
I see the patch is uploaded to bug 967277, but there still seem to be issues. Clearing ni for now.
Flags: needinfo?(21)
In addition to any changes to contacts app, I think we should investigate lowering apz.pan_repaint_interval as described in comment 6.  Even lowering it a moderate amount could have a positive impact across the whole system since many apps use these same techniques.

Would 150ms be too low?  Twice the initial fling value of 75ms?
FWIW 250ms seems very high to me as well. With sync scrolling we did 15ms so this is a massive jump.

If we're going to change it we should avoid shipping one release with the 250ms since this will be visible to some user script. We don't want them to tweak this in their app for <1.3, 1.3, and >1.3 should we tweak it after APZC all with big changes.
(In reply to Ben Kelly [:bkelly] from comment #9)
> In addition to any changes to contacts app, I think we should investigate
> lowering apz.pan_repaint_interval as described in comment 6.  Even lowering
> it a moderate amount could have a positive impact across the whole system
> since many apps use these same techniques.

If we turned on position: sticky for contacts, sms and the call log, I agree that we should changed from 40 to something like 100ms or more if we can prove that it does not have bad side effects.
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #11)
> If we turned on position: sticky for contacts, sms and the call log, I agree
> that we should changed from 40 to something like 100ms or more if we can
> prove that it does not have bad side effects.

Vivien, what value are you talking about here?  Is 40 a typo or are you referring to a different setting?  I was hoping to decrease apz.pan_repaint_interval, but it sounds like you are talking about increasing something here?

Thanks!
Attached file Prototype - remove visibility monitor (obsolete) — 
This patch removes visibility monitor all together. I'm actually seeing better fps/performance without it, than with it.

Also in this patch is the position: sticky fix.

I'm going to gather some profiles/memory measurements, and this should give us some good hints if this is a path worth exploring.
So, I don't doubt this makes scrolling faster, but please check goals (1) and (2) in comment 2.  What does this do to the total load time for reference-workload-heavy?

Unless APZC has changed the dynamics in play, I suspect it will take much longer to be able to jump to 'z' in the list with this change.  Essentially, rendering to the DOM greedily optimizes scroll speed at the cost of throttling how fast we can pull data from the contacts API cursor.
Yeah, I'm sure that there's a lot of factors that will influence this, I was trying to get to a state where we could compare the two profiles for now.

It does take a while to jump, but there's potentially other remedies - such as rendering on a setTimeout, queue, and canceling the queue when you jump. It's a lot of work either way, so we should only do this if we have better performance as far as responsiveness and memory consumption goes. I'm working on getting solid FPS/memory numbers now.
Well, we can't just cancel the load when we jump because of the whole annoyance of "we get the sort order from contacts API, but we can't get a cursor with the sort order/name at an arbitrary point" thing in contacts API.

See my PR in bug 894611 for my previous atttempt to use a queue.  It was ultimately not landed because it did not help much over the current approach, but added a lot of complexity.

I'm curious if using requestAnimationFrame() instead of timeouts would help, though.
I guess, I'm just wondering if it might be lower risk for v1.3 and quicker to try reducing the apz.pan_repaint_interval first before doing major surgery on the contacts list code.  Unfortunately, I found that it was very easy to regress our load measurements while optimizing FPS and vice verse.
Yeah, experimenting with apz.pan_repaint_interval is definitely a much safer approach for 1.3 here. I was just doing this as a poc to see what it would take, or see if it would show any potential improvements with APZ. I don't really see us being able to improve visibility handler all that much without rewriting most/all of it - and that seems way too risky for 1.3.

I'll probably take a few more measurements with this approach, then start looking at apz.pan_repaint_interval.
Comment on attachment 8370713 [details]
Prototype - remove visibility monitor

This is potentially possible to do in the future, but way too risky to do for 1.3. Going to look into some of these apz prefs and see what performs well.
Attachment #8370713 - Attachment is obsolete: true
So I've been playing with the prefs a bunch, tried twiddling various ones, but I can't really seem to find any combination that makes this work really well. In addition position: sticky seems worse in latest HEAD. I set apz.pan_repaint_interval and apz.asyncscroll.timeout at different values, but still having issues.

I suppose this bug is *really* about visibility_monitor.js though, so if we want to tweak the apz preferences, we should do so in another bug.
Did you notice any worse behaviour with the pan_repaint_interval changed? We'll probably want to reduce that regardless. The asyncscroll.timeout pref won't affect anything unless you're listening to the mozasyncscroll event which IIRC isn't available to apps.
Kevin, can you take a profile like in comment 0 and comment 2 to see if the scrollHandler() in tag_visibility_monitor is being called at the increased rate?

I don't think the apz setting is a different bug as it directly plays into the performance of the visibility monitor approach.
Kevin, please set the [ETA:<date>] in the whiteboard field. Program Management is using that tag to track 1.3 progress.
Status: NEW → ASSIGNED
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
Whiteboard: [c=handeye p=3 s= u=1.3] → [c=handeye p=3 s= u=1.3] [ETA:?]
Target Milestone: --- → 1.4 S1 (14feb)
Whiteboard: [c=handeye p=3 s= u=1.3] [ETA:?] → [c=handeye p=3 s= u=1.3] [ETA: 11/02/14]
So I'm having issues trying to profile the compositor. These measurements were taken on a Peak, and I will try to get them on a hamachi as well if that would be helpful..

apz.pan_repaint_interval @ 250ms delay: http://people.mozilla.org/~bgirard/cleopatra/#report=742feaf00cc2be58c689aa12ba5f6291581b247a&search=scrollHandler

apz.pan_repaint_interval @ 100ms delay: http://people.mozilla.org/~bgirard/cleopatra/#report=63795954454ef9e78f22e1507e62103e2fc198b8&search=scrollHandler

It does seem that visibility monitor fires much more often with the 100ms delay. Perhaps setting it to 100ms could help?
I talked to kats and he's going to reduce the delay. With 250ms we risk having the main thread go idle when we really should be using all the CPU juice to scroll.
I've tweak down the minimum paint interval and tried the performance. It may or may not be faster but certainly this remains a huge performance bottleneck.

The app is still building a large complex DOM but it's lazily filling in the text and the picture nodes. The DOM is O(n) instead of O(k) where n is the number of contacts and k is the number of contacts visible on screen at a given time. Worse while scrolling list.js is tweaking the DOM so we're reflowing a DOM of O(n+k) every scroll frame. Without using a visibily monitor we should be able to layout the page once and not have to touch it without reflowing so perform a O(n*3) reflow on page load. Instead we're performing reflow O(n+k) per frame. The difference is we're doing O(n+k) every frame instead of O(n*3) on page load. Note that k is tiny relative to k.

I'm simplifying greatly above but I don't think the Contact' app use of the visibility monitor is proving a benefit here.

So I hacked it off by giving it an window size greater then the page so have it prefill everytime at once. I get a *MUCH* better profile here (leafs are bogus here):
http://people.mozilla.org/~bgirard/cleopatra/#report=ca1249daf5f0fb2a03919d033fed1b23046cf983

I'm seeing reflows go from 300-500ms down to 30-100ms.

With this change we're mostly blocking on nsDisplayText performance.

kgrandon I'm going to leave the solution up to you but I strongly recommend that you rope in the layout team for performance advice on these reflows.

I also tried tweaking the scrollMargin/scrollDelta values but didn't see a significant improvement.
Benoit, I think this is effectively the same as what Kevin proposed in comment 13.  It moves all rendering to load time instead of incrementally at scroll time.

Again, I think we need to be careful that we don't regress our load time.  In particular the time it takes for the user to be able to click the Z letter in the list and jump to the contact at the end.  See bug 865747.

Can you please measure the difference in time between here:

  https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/list.js#L1041

And here:

  https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/list.js#L1069

Both with and without your patch?  My expectation is this will regress the time to complete a full load, thus delaying the users ability to seek.
Flags: needinfo?(bgirard)
Comment on attachment 8370713 [details]
Prototype - remove visibility monitor

Reviving this for feedback. BenWa - I tend to agree with you, but I'm sure there are *some* ways that we regress with this patch. For instance, with 2k contacts, on startup we use ~1M more ram (according to the new memory overlay in settings), but visibility monitors surpasses that while scrolling.

It is very slow to jump to the letter 'Z' with either approach. I noticed no checkerboarding/white areas with this patch, and images display instantly. There is also *0* reflows while scrolling.

BenWa - can you grab a profile of this patch and see what you think? I'm going to look at it again and measure total load time, and how we can improve responsiveness at start. If this turns out well, it may be possible to implement in 1.3 with a weeks worth of effort or so. (This may also be further improved by the work those guys are doing on the contact image thumbnails)
Attachment #8370713 - Attachment is obsolete: false
Attachment #8370713 - Flags: feedback?(bgirard)
Comment on attachment 8370713 [details]
Prototype - remove visibility monitor

I'm not qualified to give feedback on this kind of change. If you have the patch building locally you should grab profiles locally first.

I effectively disabled the contact app' use of visibility monitor use locally to prove that it was seriously hurting us. I'm not advocating that this is the fix needed for this bug I'll leave coming up with the right fix up to you.
Attachment #8370713 - Flags: feedback?(bgirard)
Flags: needinfo?(bgirard)
(In reply to Kevin Grandon :kgrandon from comment #29)
> It is very slow to jump to the letter 'Z' with either approach. I noticed no
> checkerboarding/white areas with this patch, and images display instantly.
> There is also *0* reflows while scrolling.

Kevin, can you quantify this with console.log and Date.now() instrumentation?  I am boarding a flight shortly, so can't look myself.

If we can get release drivers to verify scroll performance is more important than the jump-to-end performance described in bug 865747, then I guess I'd be ok regressing there.  I just want to have our eyes open and make a conscience decision if we go that way.
We have Chris Lee lined up to choose between scrolling and "jump to end", if it comes to that, so if there are any numbers to help, that would be great.
Comment on attachment 8370713 [details]
Prototype - remove visibility monitor

This patch got bitrotted and needs to be rebased. I will revive it shortly.
Attachment #8370713 - Attachment is obsolete: true
I'm trying to gather some numbers, but I'm having a hard time even getting jump links working with APZ in the first place. I've filed bug 970638 to track jump link problems with APZ.
Kevin, your position:sticky patch worked with jump links and APZC when I tried it.  Whats changed?
(In reply to Ben Kelly [:bkelly] from comment #31)
> (In reply to Kevin Grandon :kgrandon from comment #29)
> > It is very slow to jump to the letter 'Z' with either approach. I noticed no
> > checkerboarding/white areas with this patch, and images display instantly.
> > There is also *0* reflows while scrolling.
> 
> Kevin, can you quantify this with console.log and Date.now()
> instrumentation?  I am boarding a flight shortly, so can't look myself.
> 
> If we can get release drivers to verify scroll performance is more important
> than the jump-to-end performance described in bug 865747, then I guess I'd
> be ok regressing there.  I just want to have our eyes open and make a
> conscience decision if we go that way.

Scroll performance in general is going to be more important that jump performance given how common/often users scroll vs. jumping.   

Can someone help quantify the jump performance hit/regression?  Thanks.
(In reply to Kevin Grandon :kgrandon from comment #34)
> I'm trying to gather some numbers, but I'm having a hard time even getting
> jump links working with APZ in the first place. I've filed bug 970638 to
> track jump link problems with APZ.

Kevin, do Kats' instructions in https://bugzilla.mozilla.org/show_bug.cgi?id=970638#c1 help you continue?
(In reply to Milan Sreckovic [:milan] from comment #37)
> Kevin, do Kats' instructions in
> https://bugzilla.mozilla.org/show_bug.cgi?id=970638#c1 help you continue?

Possibly, but I am still not certain that the fix I had in mind is possible for 1.3 - it seems it may be too risky. I've also tried various approaches with changing parameters to visibility monitor, but still no luck with reducing checkerboarding. Going to look at other options today.
Whiteboard: [c=handeye p=3 s= u=1.3] [ETA: 11/02/14] → [c=handeye p=3 s= u=1.3] [ETA: 21/02/14]
(In reply to Chris Lee [:clee] from comment #36)
> Scroll performance in general is going to be more important that jump
> performance given how common/often users scroll vs. jumping.   
> 
> Can someone help quantify the jump performance hit/regression?  Thanks.


Regarding difference in numbers for jump when we remove visibility monitor:

In latest 1.3 build it takes me 4.4s to jump to the letter z with a medium reference workload. Using the same reference workload and the patch that removes visibility monitor, you are first able to jump to the letter z in about 6.0s. In either case the performance to jump time is abysmal (it should probably be instant, or less than 100ms). If we did try this approach we would be looking at a ~1.6 second regression in time to jump - which I don't think is particularly bad given it's terrible state currently.

Scrolling is guaranteed to be better though, with zero checkerboarding. I still need to do some additional memory tests, but the numbers I saw looked promising. Visibility monitor actually uses *more* memory during a scroll, than without it, but during idle visibility monitor saves some memory.
FWIW visibility monitor is the right idea. Just in it's current form it's not being used properly by the contacts app. I'd vote for removing its usage in it's current form and re-implementing how the contact app uses it properly in a future release.
(In reply to Benoit Girard (:BenWa) from comment #40)
> FWIW visibility monitor is the right idea. Just in it's current form it's
> not being used properly by the contacts app. I'd vote for removing its usage
> in it's current form and re-implementing how the contact app uses it
> properly in a future release.

I think in the long run we actually need a type of streaming list for all lists in gaia. This way we can jump to any position in the list instantly without needing to wait for the entire list to render. It's something that I've been wanting to do for a while, but we've just lacked the platform support to do it well because of missing localized sorting in IndexedDB.
Kevin, what reference workload are you testing with?  The tests in bug 865747 we're using reference-workload-heavy.  The numbers you posted in comment 39 are much lower than the 9 to 20 seconds we saw there, so I suspect you are using a smaller workload.  If possible can you measure with -heavy?
I used reference-workload-medium, but will try again with -heavy.
New numbers with a heavy workload: 

Number of contacts: 1000
Master branch: 7.7s
Removal of visibility monitor: 11.5s

For readability, my the previous medium workload results:

Number of contacts: 500
Master branch: 4.4s
Removal of visibility monitor: 6s
Chris - can you let us know if the time to scroll numbers in comment 45 are acceptable? They are pretty awful either way, and need a major overhaul in the long run. Thanks!
Flags: needinfo?(clee)
I am starting to wonder if some of the layout time differences we are seeing is due to position:sticky landing and the removal of some overflow:hidden styles.  It seems that change may have increased our memory footprint quite a bit.  See bug 970360.  Maybe we should figure out position:sticky required the removal of overflow:hidden.
Comment on attachment 8374441 [details] [review]
WIP - remove visibility monitor (master)

There is a pretty high risk in this patch, just due to the sheer size that it would be in the end. Before I spend a week working on this, I'm going to ask for 1.3 approval to ensure that it's even possible that we could land this.

Fabrice - this is only the start of the patch as it works for a plain contact list, but is going to need updates to work with gmail/facebook contact importing most likely.
Attachment #8374441 - Flags: approval-gaia-v1.3?(fabrice)
For my local testing I just made the visibility monitor viewport effectively unlimited. This probably isn't as efficient as removing the visible monitor but I saw significant improvements. This may be a good option for 1.3
(In reply to Benoit Girard (:BenWa) from comment #49)
> For my local testing I just made the visibility monitor viewport effectively
> unlimited. This probably isn't as efficient as removing the visible monitor
> but I saw significant improvements. This may be a good option for 1.3

Do you have a patch available? I also tried the same thing, but I was still seeing reflows, and checkerboarding..
This is an alternative approach depending on what Fabrice/Chris Lee says. I'm not really a fan of this approach as it increases the number of reflows on load, and yet there's still checkerboarding during scroll.

With a heavy workload we have 164 reflows instead of 116 on startup. It may be possible to remove these by tweaking visibility monitor though.
Comment on attachment 8374441 [details] [review]
WIP - remove visibility monitor (master)

Too risky for 1.3 at this point.
Attachment #8374441 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3-
Scroll-to-end numbers slow down further when maximizing the visibility monitor area:


Number of contacts: 1000
Master branch: 7.7s
Removal of visibility monitor: 11.5s
Huge visibility monitor area: 14.8s
(In reply to Fabrice Desré [:fabrice] from comment #53)
> Comment on attachment 8374441 [details] [review]
> WIP - remove visibility monitor (master)
> 
> Too risky for 1.3 at this point.

What about 1.4?  We basically have two weeks, as this is tagged as the 1.4 CS blocker.
Going to renom to discuss whether it's even possible to fix this on 1.3 - the above comments seem to imply that there's too much risk on the table to try this fix this at this point.
blocking-b2g: 1.3+ → 1.3?
Kevin

Please provide your comments on:
1) Will checker boarding be rendered incomplete if this bug is not taken?
2) Risk profile of taking it late in 1.3
Flags: needinfo?(kgrandon)
There are two potential patches here (one for removing the visibility monitor, and one for disabling it).

Disabling it is less risky - but does not fully solve checkerboarding. I had hoped that disabling it with something like bug 967292 would remove checkerboarding, but it appears that bug 967292 is no longer 1.3+.

Removing it is way more risky, but solves the checkerboarding problem. This is the patch that was deemed too risky for 1.3 - and it probably is.
Flags: needinfo?(kgrandon)
Based on Kevin's comment, I think I'm currently leaning towards not taking any patch to 1.3 at this point. We've got risk in every possible direction here, so I think sticking with what we know is the safe route.
blocking-b2g: 1.3? → backlog
I'll keep thinking about this - but if someone wants to grab it, that's fine too.

Clearing ni? on clee until we need to make a decision on time to jump-to-end vs scroll.
Assignee: kgrandon → nobody
Flags: needinfo?(clee)
I'm going to test my theory in comment 47, which should have referred to bug 942460.  I want to make sure that position:sticky fix did not introduce this regression.
Profile and video of current v1.3:

  http://people.mozilla.org/~bgirard/cleopatra/#report=0d9508e50a780928fda3aba64ae12e580d58e726
  https://www.dropbox.com/s/ek6ae6ey0i2mupq/20140219_contacts_v13_sticky.MOV

Profile and video of v1.3 with bug 942460 backed out:

  http://people.mozilla.org/~bgirard/cleopatra/#report=b66ffcf5ebaa76e4107a97485dae9d7343c14ff8
  https://www.dropbox.com/s/njcgfvx0fwudubi/20140219_contacts_v13_no_sticky.MOV

This did not help with the large blocks on javascript execution being discussed here unfortunately.  It dropped the largest block from 200ms to 180ms.

It did, however, have a huge impact on time spent in nsDisplayText.  The maximum block dropped from 290ms to 50ms.  You can see this reflected in the complete white screen shown during the scroll video with sticky enabled.  I'll post about this over in bug 967292.

While we want position:sticky in the long term, I think we should seriously consider backing out bug 942460 until we can figure out why it has such a large impact on text rendering.  The downside is that sometimes while scrolling the header may appear missing or mispositioned briefly.

Jason, Fabrice, what do you think?  I don't want to touch v1.3 without your go-ahead.
Flags: needinfo?(jsmith)
Flags: needinfo?(fabrice)
This change combined with backing out bug 967292 make scrolling more usable for me.  Its basically a compromise between the current situation and Benoit's suggestion of expanding the visibility monitor margin to cover the entire list.  Checker boarding still happens, but it is reduced.

--- a/apps/communications/contacts/js/views/list.js
+++ b/apps/communications/contacts/js/views/list.js
@@ -656,10 +656,10 @@ contacts.List = (function() {
     // onscreen() calls when adding those first contacts.
     var vm_file = '/shared/js/tag_visibility_monitor.js';
     LazyLoader.load([vm_file], function() {
-      var scrollMargin = ~~(getViewHeight() * 1.5);
+      var scrollMargin = ~~(getViewHeight() * 4.0);
       // NOTE: Making scrollDelta too large will cause janky scrolling
       //       due to bursts of onscreen() calls from the monitor.
-      var scrollDelta = ~~(scrollMargin / 15);
+      var scrollDelta = 0;
       monitor = monitorTagVisibility(scrollable, 'li', scrollMargin,
                                      scrollDelta, onscreen, offscreen);
     });
Would it be possible to provide a setting in the manifest which disables APZC on an app-by-app basis?

Over all I'm really concerned with how we can ship v1.3 with the contacts app in this situation.  We just don't have a real solution for contacts+APZC that is low risk enough for v1.3.  IMO, the app is more usable without APZC at the moment.
(In reply to Ben Kelly [:bkelly] from comment #62)

> Jason, Fabrice, what do you think?  I don't want to touch v1.3 without your
> go-ahead.

Can you provide a test build to QA to smoketest it side by side with the current 1.3?
Flags: needinfo?(fabrice)
I'll do this tomorrow.  I jist need to figure out if I need to do anything special to generate a build like the ones QA uses.
(In reply to Ben Kelly [:bkelly] from comment #64)
> Would it be possible to provide a setting in the manifest which disables
> APZC on an app-by-app basis?
> 
> Over all I'm really concerned with how we can ship v1.3 with the contacts
> app in this situation.  We just don't have a real solution for contacts+APZC
> that is low risk enough for v1.3.  IMO, the app is more usable without APZC
> at the moment.

I don't think we've got an option on the table to ship without APZC. It was deemed as a CS blocker for 1.3 to my understanding.

(In reply to Ben Kelly [:bkelly] from comment #66)
> I'll do this tomorrow.  I jist need to figure out if I need to do anything
> special to generate a build like the ones QA uses.

If it's Gaia-based, then all we need is a github branch with the specified Gaia changes.
Flags: needinfo?(jsmith)
(In reply to Jason Smith [:jsmith] from comment #67)
> I don't think we've got an option on the table to ship without APZC. It was
> deemed as a CS blocker for 1.3 to my understanding.

I understand, but its hard for me to accept shipping APZC if it introduces regressions.  It seems smooth fps is less useful if you can't see what you are scrolling due to checkerboarding.
 
> If it's Gaia-based, then all we need is a github branch with the specified
> Gaia changes.

Here is a branch that backs out bug 942460 and applies the patch in comment 63:

  https://github.com/wanderview/gaia/tree/contacts-qa-test-967884

I also made a branch that backs out bug 942460, applies the patch from comment 63, and chunks onscreen rendering using requestAnimationFrame.  It might be slightly better:

  https://github.com/wanderview/gaia/tree/contacts-qa-test-967884-v2

Both of these branches are based on v1.3.

Adding qawanted to compare to latest v1.3 branch.
Assignee: nobody → bkelly
Keywords: qawanted
(In reply to Ben Kelly [:bkelly] from comment #68)
> (In reply to Jason Smith [:jsmith] from comment #67)
> > I don't think we've got an option on the table to ship without APZC. It was
> > deemed as a CS blocker for 1.3 to my understanding.
> 
> I understand, but its hard for me to accept shipping APZC if it introduces
> regressions.  It seems smooth fps is less useful if you can't see what you
> are scrolling due to checkerboarding.

I'd suggest raising this on b2g-release-drivers.
This is blocking QC CS 1.4.
blocking-b2g: backlog → 1.4+
If 1.4 is the new target then I think we should seriously consider re-writing the contacts list code to use the approach described by roc here:

  http://robert.ocallahan.org/2014/02/implementing-virtual-widgets-on-web.html

It would be a huge amount of effort requiring DataStore sync to IDB, changes to search, and export selection, but its probably our best bet for making contacts app content fast enough for APZ scrolling.
See Also: → 975680
(In reply to Milan Sreckovic [:milan] from comment #70)
> This is blocking QC CS 1.4.

No it is not - QC needs to nominate individual dependencies, not a giant meta bug, to determine if something is a FC or CS blocker for a release.
blocking-b2g: 1.4+ → 1.4?
Perhaps more detail is necessary.  This is part of a solution to the blocker bug.  It needs to be solved if the blocker bug 942750 is to be solved.  If it isn't marked as 1.4+, people are not allowed to work on it. How do you suggest we deal with this, other than marking it as 1.4+, other than having a better explanation than I put in the first time?
blocking-b2g: 1.4? → 1.4+
Kevin is focusing on addressing the checkerboarding in contacts.  I'm moving this bug back to him.
Assignee: bkelly → kgrandon
Whiteboard: [c=handeye p=3 s= u=1.3] [ETA: 21/02/14] → [c=handeye p=3 s= u=1.4]
Target Milestone: 1.4 S1 (14feb) → ---
Going to remove qawanted as I think we've already discovered per the related bug 977680 that including position:sticky actually hurts scrolling perf.
Keywords: qawanted
Comment on attachment 8374441 [details] [review]
WIP - remove visibility monitor (master)

Obsoleting in favor of bug 975680.
Attachment #8374441 - Attachment is obsolete: true
Comment on attachment 8375841 [details] [review]
Alternative Approach - Disable visibility monitor entirely.

Obsoleting in favor of bug 975680.
Attachment #8375841 - Attachment is obsolete: true
Depends on: 975680
Blocks: 982210
I am currently looking at solving this with the work being done in bug 975680. Unfortunately that's turning out to be quite the rabbit hole and I don't see that making 1.4.

I believe that some of the new blockers of bug 982210 may improve this as well. My recommendation is to solve all dependent bugs on 982210, then revisit this again as doing that should improve the performance here.

I would actually recommend making this a meta bug and dependent on: 976605 981899 982205 982279.
(In reply to Kevin Grandon :kgrandon from comment #79)
> I am currently looking at solving this with the work being done in bug
> 975680. Unfortunately that's turning out to be quite the rabbit hole and I
> don't see that making 1.4.
> 
> I believe that some of the new blockers of bug 982210 may improve this as
> well. My recommendation is to solve all dependent bugs on 982210, then
> revisit this again as doing that should improve the performance here.
> 
> I would actually recommend making this a meta bug and dependent on: 976605
> 981899 982205 982279.

Kevin,

So are we going for the suggestion and proceeding with it? QC is expecting this one to be fixed before 3/24. If we make this meta, seems like all the depending bugs also have to be 1.4 blockers. Wonder if we can get the ETA for this one in day or two.
Flags: needinfo?(jcheng)
If this turns into a meta, which I think it is, then it probably shouldn't have an ETA as no work will be done on this bug. If it's possible to solve 976605 981899 982205 982279 in 1.4, then they should probably be nomed for 1.4?. (I don't know if it's actually possible to fix those within the next 5 days though)
HI Ivan, i don't see this blocking bug 960372 though. can you confirm this really blocks our partner? Thanks
Flags: needinfo?(jcheng) → needinfo?(itsay)
(In reply to Joe Cheng [:jcheng] from comment #82)
> HI Ivan, i don't see this blocking bug 960372 though. can you confirm this
> really blocks our partner? Thanks

Hi Joe,

This bug blocks 942750 and 942750 blocks 960372. Also, This bug is one of bug in the list specified by our partner that they want know the status and ETA. But as Kevin mentioned, if this one will be the meta, we need to take a look its depending bug and see if all of them have to be nominated.
Flags: needinfo?(itsay) → needinfo?(jcheng)
There are a number of bugs that bug 942750 depends on, but they don't necessarily block that bug, since it is performance related, and performance improvements can at times come from different places.  I would consider only bugs explicitly blocking bug 960372 as QC 1.4 FC blockers.
Discussed with Milan in IRC - this isn't a QC blocker anymore, but we should evaluate if this blocks any of the checkerboarding per app bugs or not. If it does block any of those bugs, then this should remain as a 1.4+ bug. If it doesn't, then we should not block on this.
blocking-b2g: 1.4+ → 1.4?
Kevin & Mason,

Per Jason's comment 85, does this issue block fixing the existing checkerboarding per app bugs?

Thanks,
Mike
Flags: needinfo?(mchang)
Flags: needinfo?(kgrandon)
It is likely that this would help reduce checkerboarding, but it is not the only fix that we'd need. Again, my recommendation is that we solve bug 976605, bug 981899, bug 982279 first - then revisit to see if this issue remains.
Assignee: kgrandon → nobody
Flags: needinfo?(kgrandon)
Dropping 1.4 nom in favor of 1.5 per Kevin's comment 87.
blocking-b2g: 1.4? → 1.5?
Flags: needinfo?(mchang)
Whiteboard: [c=handeye p=3 s= u=1.4] → [c=handeye p=3 s= u=]
Flags: needinfo?(jcheng)
I don't think we should block on this for 1.5.  Francisco and Jose are pursuing an effort to re-architect contacts data model and windowing.  I think we need to pursue that first as the current data model is a core problem that makes this bug so difficult.
blocking-b2g: 1.5? → ---
Priority: P1 → --
Whiteboard: [c=handeye p=3 s= u=] → [c=handeye p=5 s= u=]
Priority: -- → P3
No longer depends on: 975680
See Also: → 976605
See Also: → 981899, 982279
(In reply to Ben Kelly [:bkelly] from comment #89)
> I don't think we should block on this for 1.5.  Francisco and Jose are
> pursuing an effort to re-architect contacts data model and windowing.  I
> think we need to pursue that first as the current data model is a core
> problem that makes this bug so difficult.

Hi all,

as Ben commented, we are in the middle of proposing a new data model to speedup the contacts list based on the idea of Datastores and local (specific, precalculated and indexed) data.

We are putting our efforts in bug 968098, but we are not committing for 1.5 since we are still not sure if we will make it for the deadline.

Kevin, this we discussed that during the meeting we tried to setup in the Comms workweek, I really would like your input in our ideas, so perhaps we could review the plan with you.
Sounds good. I'm currently at a work week, but will follow-up when I have time. In the meantime I will follow that bug and any dependent bugs.
Apparently visibility monitor is no longer needed, and we are removing it from the gallery app. I would suggest that we do the same for the contacts app. This would result in a longer delay for jump links to work, but that doesn't seem so bad as it's already pretty slow.
See Also: → 967089
Contacts uses visibility monitor for more than removing images.  For example, its also used for updating checkbox state in selection mode (vs setting checkboxes for 1000 contacts all at once).

Still, it would be interesting to move images out of visibility monitor.  Note, this will require using img elements instead of CSS background-images.  It seems like this may make bug 981899 more significant.

Overall, I think moving images out of visibility monitor should be a separate bug.
Firefox OS is not being worked on
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: