1068888 - [Rocketbar] Defer saving of place until loadend

Status: RESOLVED FIXED

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

Description

[Kevin Grandon :kgrandon]

Spinning this off of bug 1050510 because it seems like it may be a slightly different issue, but contributing to the CPU usage we're seeing there.

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Dale - what do you think of something like this?

Comment 2

[Dale Harvey (:daleharvey)]

Comment on attachment 8490997 [details] [review]
Github pull request

I do agree we should debounce the changes, for example getting a bunch of icon definitions in at the same time

However there are a few problems doing it this way, first in places is a singleton so any reference to local state is going to need to worry about what url the event is being referred to |this._currentPlaceEdits| when switching between apps.

Another is that we cant just write things on loadend, its pretty trivial to write a website that wont ever trigger a loadend event (guardian.co.uk for example), also dealing with this in https://bugzilla.mozilla.org/show_bug.cgi?id=1064920

Comment 3

[Kevin Grandon :kgrandon]

Seems like we should make a URL mapping then, and potentially store multiple records locally. Also what we can do is perform a setTimeout in addition to the loadend. So, basically store multiple records, save either onTimeout after locationchange, or loadend.

I think this is potentially fine, until we have an evangelism or platform fix here. What do you think Dale, would you be ok with this approach?

Comment 4

[Dale Harvey (:daleharvey)]

Yup I think that approach would work good

Comment 5

[Kevin Grandon :kgrandon]

Comment on attachment 8490997 [details] [review]
Github pull request

Dale - I've updated this to save after a timeout or load end event. It also handles multiple places. What do you think?

Comment 6

[Dale Harvey (:daleharvey)]

Comment on attachment 8490997 [details] [review]
Github pull request

This looks good, I do think we want to be more careful about not losing data though, we can get rid of |defaultPlace()| and only merge the keys which we have set is likely the cleanest way, but up to you, going to need some tests to make sure we dont overwrite / lose places data as well I think since it can be tricky / easy to miss

Comment 7

[Kevin Grandon :kgrandon]

Sounds good. The unit test will probably be something like, receive a titlechange after a loadend event, and verify we eventually save that as well. Working on this now.

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8490997 [details] [review]
Github pull request

Hey Dale - hopefully we're getting closer, thanks for the reviews. I've put the updates into a separate commit for ease of review. This adds some tests to ensure we don't lose data and hit the debouces. This can now store multiple concurrent debounces as well for different URLs as well. Let me know what you think.

Comment 9

[Dale Harvey (:daleharvey)]

Comment on attachment 8490997 [details] [review]
Github pull request

This is looking good and working well for me, I commented on github I think theres another null check needed before landing, but rest looks good, thanks

Comment 10

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/a2453cf7b44f553761807208269c265f07423dd8

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 8490997 [details] [review]
Github pull request

I would like to see this uplifted to 2.1 for performance improvements.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature implementation.
[User impact] if declined: Worse performance than if we take it.
[Testing completed]: Manual and unit testing.
[Risk to taking this patch] (and alternatives if risky): Contained to system browser history, so should be relatively low risk.
[String changes made]: No.

Comment 12

[[:fabrice] Fabrice Desré]

(In reply to Kevin Grandon :kgrandon from comment #11)
> Comment on attachment 8490997 [details] [review]
> Github pull request
> 
> I would like to see this uplifted to 2.1 for performance improvements.
> 
> [Approval Request Comment]
> [Bug caused by] (feature/regressing bug #): Feature implementation.
> [User impact] if declined: Worse performance than if we take it.

For sure I would not approve if you said that this regressed performance ;)
What is the gain here? 1% ? 10%? of what?

Comment 13

[Kevin Grandon :kgrandon]

(In reply to Fabrice Desré [:fabrice] from comment #12)
> For sure I would not approve if you said that this regressed performance ;)
> What is the gain here? 1% ? 10%? of what?

It depends on what kind of browser events the site was generating, so it's hard to give concrete performance numbers here. Before on average we were probably hitting datastore ~6-7 times per site load, now we should generally just hit it once. There is a profile in bug 1050510 comment 29 which shows each editPlace taking around ~15ms.

If we save ~5 places.js calls per page load, then we save 60ms of CPU time per page load.

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 8490997 [details] [review]
Github pull request

Not bad enough to uplift.

Comment 15

[Kevin Grandon :kgrandon]

(In reply to Fabrice Desré [:fabrice] from comment #14)
> Comment on attachment 8490997 [details] [review]
> Github pull request
> 
> Not bad enough to uplift.

Humm, 60ms per page load seems pretty awful to me - and this is probably the biggest perf improvement we could make right now. It also could be considered a perf regression.

If it turns out that we do get any blockers, or bug 1050510 becomes blocking, then we should reconsider uplift.