Closed Bug 1170724 Opened 9 years ago Closed 9 years ago

Autophone - 2015-05-20 Throbber start regression in S1S2 on fx-team

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox40 unaffected, firefox41 fixed, firefox42 fixed, fennec41+)

Status:
RESOLVED FIXED
Milestone:
Firefox 42
Tracking Flags:
Tracking Status
firefox40 --- unaffected
firefox41 --- fixed
firefox42 --- fixed
fennec 41+ ---

People

(Reporter: bc, Assigned: mcomella)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: regression)

Attachments

(2 files, 5 obsolete files)

MozReview Request: Bug 1170724 - Only update search engine bar when necessary. r=sebastian
40 bytes, text/x-review-board-request
sebastian
: review+
ritu
: approval-mozilla-beta+
Details
Profile (Left = with search engine bar; Right = post-patch)
483.51 KB, image/png
Details
NI self to check this out.
Flags: needinfo?(michael.l.comella)
Sebastian, the search engine bar is suspected here - if you have a few cycles, can you check it out?
Flags: needinfo?(michael.l.comella) → needinfo?(s.kaspari)
(In reply to Michael Comella (:mcomella) from comment #2) > Sebastian, the search engine bar is suspected here - if you have a few > cycles, can you check it out? Sure! Can you help me to understand what the actual regression is? I'm not sure yet what I should check out here (Throbber start? S1S2?).
Flags: needinfo?(s.kaspari) → needinfo?(michael.l.comella)
Discussed in person.
Flags: needinfo?(michael.l.comella)
tracking-fennec: ? → 41+
status-firefox40: --- → unaffected
Mike or Sebastian, can one of you own this?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(michael.l.comella)
Assignee: nobody → s.kaspari
Flags: needinfo?(s.kaspari)
Looking at the past 30 days [1], it seems the regression is still in place (i.e. the baseline was 1200 before my patch and 1400 after, the baseline now is roughly 1400, with some improvements in the last week). [1]: http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstart/local-blank/norejected/2015-06-08/2015-07-08/notcached/noerrorbars/standarderror/notry
I'll take a stab at this first thing in the morning.
Assignee: s.kaspari → michael.l.comella
Flags: needinfo?(michael.l.comella)
(In reply to Michael Comella (:mcomella) from comment #9) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9de319a3e420 A try build that just removed the SearchEngineBar from BrowserSearch. I have a suspicion it could also be that we switch to setting the window background when BrowserSearch is shown.
Results (4.4 second result): Baseline (comment 16): Nexus 4: 972±34 Nexus 5: 643±12 Nexus 7: 889±178 Search removed (comment 15): Nexus 4: 957±20 Nexus 5: 707±45 Nexus 7: 735±10 --- With the search engine bar removed, the only significant improvement is on the N7 (with a large error bar with search enabled!), while the N5 actually is worse with search removed (but within the error range). Bob, how significant are these changes? Is this something I should be concerned about? And are these units in ms?
Flags: needinfo?(bob)
Units are ms. I can't tell from your runs what is going on. Can you attach the patch that you want to test and I will run some try runs to figure it out.
Flags: needinfo?(bob)
Bug 1170724 - Remove SearchEngineBar from testAddSearchEngine. I just want to build please.
Attachment #8634157 - Attachment description: Remove SearchEngineBar from testAddSearchEngine → Patch 2 - Remove SearchEngineBar from testAddSearchEngine
Attachment #8634157 - Attachment is obsolete: false
I did two runs: 1 from tip of mozilla-inbound at the time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ad2df2b2617&exclusion_profile=false&filter-searchStr=android the other from the same tip with the two patches applied: https://treeherder.mozilla.org/#/jobs?repo=try&revision=686cb09e7d08&exclusion_profile=false I repeated the tests 3 times for a total of 24 observations per point. Pretty flat for throbber start as in local blank where the changes are within one stderr usually: http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstart/local-blank/norejected/2015-07-15/2015-07-15/notcached/errorbars/standarderror/try but... significant regression in throbber start local twitter on nexus s: http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstart/local-twitter/norejected/2015-07-15/2015-07-15/notcached/errorbars/standarderror/try somewhat regression in throbber start remote nytimes on nexus 5 http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstart/remote-nytimes/norejected/2015-07-15/2015-07-15/notcached/errorbars/standarderror/try mixed bag on remote nytimes throbber stop with a slight improvement on nexus s and a regression on nexus 4 http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstop/remote-nytimes/norejected/2015-07-15/2015-07-15/notcached/errorbars/standarderror/try and a completely unreliable remote throbber stop on nexus s http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstop/remote-twitter/norejected/2015-07-15/2015-07-15/notcached/errorbars/standarderror/try I would say the patches do not improve the situation.
Flags: needinfo?(bob)
Attached image Profile (Left = with search engine bar) (obsolete) —
The profile indicates we spend slightly longer in measuring but much, much more time drawing than before. My best guess: To avoid overdraw of the search engine bar, I used a white background in the window and drew everything on top of it with transparent backgrounds - maybe these are actually more expensive than overdraw.
Best guess in comment 24 seems incorrect. As it turns out, the XML initially inflates a visible SearchEngineBar but then we call setVisibility(GONE) when updating the search engines in onViewCreated because we have not received any search engines and with a count of zero, we hide the search engine bar. If drawing occurs before onViewCreated is called, this would cause a third layout and draw. So we draw (at least) once without the search engine bar, then we relayout and draw again once the search engines are received which causes a big UI hiccup (likely because with two list views, there are a lot of items to draw and objects to allocate) and may be the cause of this regression.
We were updating the search engine bar every time the view was created, which is inefficient because we already have a default state which we laid out and drew. Since there were no search engines at the time, the search engine bar would be hidden and another layout and draw would occur. Finally, when the search engines arrived asynchronously from Gecko, we'd lay out and draw the search engine bar (again). Obviously, this had had some perf implications. Instead, now we only update the search engine bar if we explicitly choose not to update the search engines (in onResume), or when the search engine data arrives. The default state is to draw the empty search engine bar (including magnifying glass label), to avoid a layout. We could use INVISIBLE, but then the initial list of non-search engine results will look misplaced. When the no search engines are returned (i.e. the user deletes them all), the empty search engine bar will be briefly shown before hiding. Once gecko is loaded on my N4, the search engines are received so quickly, the empty search engine bar flicker only makes a minimal impression, no different from the search engines appearing asynchronously. The best way to improve this is probably to cache the search engines in Java (bug 1186703).
Bob, can I get autophone test results for the patch in comment 27?
Flags: needinfo?(bob)
:mcomella: sure, I'll do a try run tonight with a tip build without the patch and then one with it so we can get a clear comparison.
Are these changes significant, Bob? I would expect a slight regression because we're creating a new ListView hierachy/adapter system, which requires more layout overhead. If not, do you feel we should call this fixed?
Flags: needinfo?(bob)
It is hard to say with just these two builds. If the new patch is an improvement in approach/design/code I would say it would be ok to accept the changes. If the new patch isn't an improvement in approach/design/code I don't see that it makes that much difference. It looks like we have improved the original work but not really regained the losses. I'd be willing to just call it a day and move on.
Flags: needinfo?(bob)
Comment on attachment 8634153 [details] MozReview Request: Bug 1170724 - Only update search engine bar when necessary. r=sebastian Bug 1170724 - Only update search engine bar when necessary. r=sebastian We were updating the search engine bar every time the view was created, which is inefficient because we already have a default state which we laid out and drew. Since there were no search engines at the time, the search engine bar would be hidden and another layout and draw would occur. Finally, when the search engines arrived asynchronously from Gecko, we'd lay out and draw the search engine bar (again). Obviously, this had had some perf implications. Instead, now we only update the search engine bar if we explicitly choose not to update the search engines (in onResume), or when the search engine data arrives. The default state is to draw the empty search engine bar (including magnifying glass label), to avoid a layout. We could use INVISIBLE, but then the initial list of non-search engine results will look misplaced. When the no search engines are returned (i.e. the user deletes them all), the empty search engine bar will be briefly shown before hiding. Once gecko is loaded on my N4, the search engines are received so quickly, the empty search engine bar flicker only makes a minimal impression, no different from the search engines appearing asynchronously. The best way to improve this is probably to cache the search engines in Java (bug 1186703).
Attachment #8634153 - Attachment description: MozReview Request: Bug 1170724 - Remove SearchEngineBar from BrowserSearch. → MozReview Request: Bug 1170724 - Only update search engine bar when necessary. r=sebastian
Attachment #8634153 - Flags: review?(s.kaspari)
Sebastian, if you have additional simple ways to improve the perf of this, let me know! Otherwise, let's land this.
Attachment #8634153 - Flags: review?(s.kaspari) → review+
Comment on attachment 8634153 [details] MozReview Request: Bug 1170724 - Only update search engine bar when necessary. r=sebastian https://reviewboard.mozilla.org/r/13337/#review12653 Ship It!
(In reply to Michael Comella (:mcomella) from comment #34) > Sebastian, if you have additional simple ways to improve the perf of this, > let me know! Otherwise, let's land this. Not directly. You mention above that we change visibility twice and therefore do multiple measurements/draws etc. - Would there be any benefit from doing it the other way around? Setting GONE in the layout and setting it to VISIBLE as soon as we have search engines? Or would it make sense to start from INVISIBLE (to not re-measure), because we know there are going to be some search engines most of the time, and then switch to VISIBLE or GONE as soon as we know? But I guess there's not much to gain from that? It'll probably interesting to see if switching to RecyclerView in bug 1184211 does change anything, but I guess it won't.
(In reply to Sebastian Kaspari (:sebastian) from comment #36) > Would there be any benefit > from doing it the other way around? Setting GONE in the layout and setting > it to VISIBLE as soon as we have search engines? This would require a full relayout so is less efficient. > Or would it make sense to > start from INVISIBLE (to not re-measure), because we know there are going to > be some search engines most of the time, and then switch to VISIBLE or GONE > as soon as we know? This causes the list of result items before search engines (e.g. history, bookmarks) to not fill the entire screen which looks really wrong, though it's likely more efficient.
url: https://hg.mozilla.org/integration/fx-team/rev/723e4ab1ce04c65d9b72a2072a072078b99afb43 changeset: 723e4ab1ce04c65d9b72a2072a072078b99afb43 user: Michael Comella <michael.l.comella@gmail.com> date: Wed Jul 22 16:40:50 2015 -0700 description: Bug 1170724 - Only update search engine bar when necessary. r=sebastian We were updating the search engine bar every time the view was created, which is inefficient because we already have a default state which we laid out and drew. Since there were no search engines at the time, the search engine bar would be hidden and another layout and draw would occur. Finally, when the search engines arrived asynchronously from Gecko, we'd lay out and draw the search engine bar (again). Obviously, this had had some perf implications. Instead, now we only update the search engine bar if we explicitly choose not to update the search engines (in onResume), or when the search engine data arrives. The default state is to draw the empty search engine bar (including magnifying glass label), to avoid a layout. We could use INVISIBLE, but then the initial list of non-search engine results will look misplaced. When the no search engines are returned (i.e. the user deletes them all), the empty search engine bar will be briefly shown before hiding. Once gecko is loaded on my N4, the search engines are received so quickly, the empty search engine bar flicker only makes a minimal impression, no different from the search engines appearing asynchronously. The best way to improve this is probably to cache the search engines in Java (bug 1186703).
https://hg.mozilla.org/mozilla-central/rev/723e4ab1ce04
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Context for additional improvements: it's strange an extra UI element, which doesn't need to be present to type in a url, slows down the page load times. If I had to guess, it's probably that the UI thread is used to type via the keyboard into the url bar and we're also hammering the UI thread for this new UI element. We can audit the code for slow UI thread calls but I don't think we do anything expensive so I'm not sure there's a way to improve the timing further. We can see what RecyclerView does over in bug 1184211.
Comment on attachment 8634153 [details] MozReview Request: Bug 1170724 - Only update search engine bar when necessary. r=sebastian Approval Request Comment [Feature/regressing bug #]: bug 1137483 [User impact if declined]: There will be a slight perf regression for showing the search screen with the addition of the search engine bar, and thus potentially the amount of time it takes the user to load a page. [Describe test coverage new/current, TreeHerder]: On Nightly for a bit, tested locally [Risks and why]: We move the update code to occur immediately (if we have the information) or after we've retrieved the information asynchronously from Gecko. Since we're updating the information in specific cases, it's possible we'll not update the screen in a particular situation and the state will be distorted. This is unlikely, however, since we play the code in an if/else block (as opposed to specific cases in if/elseif/...). [String/UUID change made/needed]: None
Attachment #8634153 - Flags: approval-mozilla-beta?
Comment on attachment 8634153 [details] MozReview Request: Bug 1170724 - Only update search engine bar when necessary. r=sebastian Fix has been in Nightly for over two weeks and the patch is a one-line change. Seems safe to uplift to m-b.
Attachment #8634153 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Bob, can you please verify that this issue is fixed on a latest Nightly build? Thanks!
Flags: needinfo?(bob)
As I mentioned earlier with regard to the try builds, the patches didn't have a measurable effect at least partly due to the increased noise in the throbber start measurement. While the regression was clear, the landing of the patch in this bug and in bug 1184211 comment 62 don't change the behavior appreciably. Sorry.
Flags: needinfo?(bob)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: