819628 - Use incremental commits to stores when synchronizing calendars.

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Calendar, defect, P4)

Description

[James Lal [:lightsofapollo]]

Brief background on our current sync structure:

1- We authorize account and save credentials (one transaction)
2- We sync calendars adding/removing as needed (all one transaction for all changes)

3- Per calendar we request events and sync them with the ones currently present via etag comparison all changes per calendar are in a single transaction.

---

This bug is to break up the event sync (3) into multiple commits based on the volume of changes. about:memory indicates most memory usage is in the worker (15mb~ on my test vs 5mb~) so we we likely see some benefit from doing this (as we can free records from memory much sooner) but it will be less then further optimizing the worker.

The benefits here are we will be more likely to sync larger calendars (as each transaction is broken up into smaller chunks) and in the case of memory related crashes further sync attempts will consume less (assuming we have persisted at least one chunk per transaction). For greatest effect this bug needs to be completed with 819519.

Implementation guidelines:

- Each transaction must consist of the complete details of a given event (alarms, icalComponent, busytimes, frozen recurring iterator). 

- We must not update the ctag on the calendar (when the ctag is updated we will not attempt to sync we use it to indicate we are in the same state as the server) unless all transactions complete successfully. In the case of a single transaction failure we continue (non-fatal) but we could attempt to sync again (or simply notify the user/wait for next sync).

- Need to determine best number to commit in single transaction, each operation is fairly expensive but the memory overhead is likely more expensive for huge calendars so we need to find the right balance.

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

BB+, P3 for performance

Comment 2

[Dietrich Ayala (:dietrich)]

Blocking- for not enough information about impact on user. We can't block on every performance issue. Please renominate if there's specific problem that makes the app performance unusable.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Dietrich Ayala (:dietrich) from comment #2)
> Blocking- for not enough information about impact on user. We can't block on
> every performance issue. Please renominate if there's specific problem that
> makes the app performance unusable.

True, although we should get more information here.

James - Can you be more specific on end-user impacts in terms of performance? Basically, exact user flows that would be affected and how much.

Comment 4

[James Lal [:lightsofapollo]]

As I said earlier its going to vary but on slower connections this is likely to greatly improve performance from the users perspective as they will see events prior to the entire request completing. I have a patch up for review in the CalDAV lib now and this should land in gaia this week.

Comment 5

[James Lal [:lightsofapollo]]

This could also make a big difference with crashes. Even if we crash during the first sync we can potentially complete it by syncing multiple times.

I think we should b+ this bug given the number of OOM crashes we have seen recently.

Comment 6

[Tony Chung [:tchung]]

(In reply to James Lal [:lightsofapollo] from comment #5)
> This could also make a big difference with crashes. Even if we crash during
> the first sync we can potentially complete it by syncing multiple times.
> 
> I think we should b+ this bug given the number of OOM crashes we have seen
> recently.

bug 826872 for example.

Comment 7

[James Lal [:lightsofapollo]]

After running DMD and speaking with jlebar this probably will do little to resolve the OOM issues as they are related to XHR (response bodies) and not to the database. 

This is still a nice to have but I am not sure we need it given it is very unlikely to improve the OOM situation greatly until we resolve the problems with XHR.

Comment 8

[Jason Smith [:jsmith]]

Let's just unnom in that case. No need to send through triage again.

Comment 9

[James Lal [:lightsofapollo]]

This definitely is required for huge calendars, we have resolved (in calendar & in gecko) the largest memory consumption issues but we still have run out of memory on massive calendars (on the main thread) because we are buffering all the data until the first sync (per calendar) is complete.

I really don't think this blocks, out of all the accounts I have found only one can actually crash the calendar on the latest build with the initial version of the workers patch.

Comment 10

[David Scravaglieri [:scravag]]

James, could you investigate on this ?

Comment 11

[James Lal [:lightsofapollo]]

Sorry for not responding... Not sure what we need to investigate now as per comment #9 we need this for syncing accounts with many events on many calendars. 

The threshold will vary but this should help us free up memory to handle all sizes of calendars. In addition we could limit the number of calendars we sync concurrently.

noming for visibility but we should target some release to fix this

Comment 12

[Ganesh Ghosh]

Attachment: Uses incremental commits to stores when synchronizing calendars

Comment 13

[Ganesh Ghosh]

Attachment: Bug 819628 - real patch, actually commits incrementally

Comment 14

[James Lal [:lightsofapollo]]

Comment on attachment 775245 [details] [diff] [review]
Bug 819628 - real patch, actually commits incrementally

Please attach a pull request- I cannot review this as a single commit.

Comment 15

[Ganesh Ghosh]

Attachment: Bug 819628 - Use incremental commits to stores when synchronizing calendars.

Comment 16

[James Lal [:lightsofapollo]]

Comment on attachment 775726 [details] [diff] [review]
Bug 819628 - Use incremental commits to stores when synchronizing calendars.

decent first round- see my review comments on github and reset the review flag to ? when we are ready for another round.

Comment 17

[Ganesh Ghosh]

Attachment: Incrementally commits without flags

Comment 18

[James Lal [:lightsofapollo]]

Comment on attachment 776703 [details] [diff] [review]
Incrementally commits without flags

closer- but see my GH comments

Comment 19

[Ganesh Ghosh]

Attachment: Incrementally commits without resetting objects

Comment 20

[Ganesh Ghosh]

Attachment: Incrementally commits without resetting objects and some style changes

Comment 21

[James Lal [:lightsofapollo]]

Comment on attachment 777453 [details] [diff] [review]
Incrementally commits without resetting objects and some style changes

Looks pretty good but see my comments on GH and remember about the complete/error/abort? events for the final review.

Comment 22

[Ganesh Ghosh]

Attachment: Incrementally commits and passes all tests

Comment 23

[James Lal [:lightsofapollo]]

Comment on attachment 778022 [details] [diff] [review]
Incrementally commits and passes all tests

right direction still but more to go

Comment 24

[Ganesh Ghosh]

Attachment: General Improvements to Tests

Comment 25

[Ganesh Ghosh]

Attachment: General Improvements to Tests

Comment 26

[James Lal [:lightsofapollo]]

Comment on attachment 779489 [details] [diff] [review]
General Improvements to Tests

Looks almost the same as before? Did you forget to push a commit?

Comment 27

[Ganesh Ghosh]

Attachment: Incrementally commits and passes all tests

Comment 28

[Ganesh Ghosh]

Attachment: Restructures commit tests completely, removes callback from commitGroup and emit end in manager

Comment 29

[James Lal [:lightsofapollo]]

Comment on attachment 781081 [details] [diff] [review]
Restructures commit tests completely, removes callback from commitGroup and emit end in manager

see github

Comment 30

[Ganesh Ghosh]

Attachment: adds event api

Comment 31

[James Lal [:lightsofapollo]]

Comment on attachment 782950 [details] [diff] [review]
adds event api

Looks better! Please address / argue all of my comments before flagging for a final review... I have not looked closely at the tests yet so I may have some more comments there.

Comment 32

[Ganesh Ghosh]

Attachment: Adds event api with complete, error and abort

Comment 33

[Ganesh Ghosh]

Attachment: Incrementally commits and passes all tests

Comment 34

[James Lal [:lightsofapollo]]

Comment on attachment 784639 [details] [diff] [review]
Incrementally commits and passes all tests

We are very close here... I found a bug but mostly this is all style issues I have not tried this yet on my device with a large set of data.. I will do that later today and may have some more comments then.

Comment 35

[Ganesh Ghosh]

Attachment: Incrementally commits and passes all tests

Comment 36

[James Lal [:lightsofapollo]]

Comment on attachment 787155 [details] [diff] [review]
Incrementally commits and passes all tests

[recap from in person conversation]

This is soooo close.

In the case of syncing a calendar for the first time this works really well.

There is some bustage when trying to expand a recurring event that never ends though. What It looks like there are two events we expect to be fired from the caldav service: eventComplete and event. In the case where we want to expand a recurring event we don't fire these right now.

To reproduce this problem you can create a recurring event with no end ( Every Saturday of the month, etc...) then go forward 6-8 months in time and you should see erorrs in logcat and really weird behavior in calendar like duplications of the events.

Comment 37

[Ganesh Ghosh]

Attachment: Expands infinite events correctly

Comment 38

[James Lal [:lightsofapollo]]

Comment on attachment 789233 [details] [diff] [review]
Expands infinite events correctly

in master https://github.com/mozilla-b2g/gaia/commit/8a91b987b57e01d03eded32c33a68595bd0949cb

Comment 39

[James Lal [:lightsofapollo]]

backed out of master/1.2 for delete breakage.

Comment 40

[James Lal [:lightsofapollo]]

oops...

reverted here: b9b99e5cd922d7cdaa5574b553d83cfac99c3bff

Comment 41

[Dylan Oliver [:doliver]]

This fix was reverted and no longer meets blocking criteria. Will try again in a future release.

Comment 42

[Mike Lee [:mlee]]

(In reply to Dylan Oliver [:doliver] from comment #41)
> This fix was reverted and no longer meets blocking criteria. Will try again
> in a future release.

Dylan, is this still targeted for 1.3? If not please minus this.

Comment 43

[Dylan Oliver [:doliver]]

Cleared blocking flag - this is not required for 1.3.

Comment 44

[Joe Cheng [:jcheng] (please needinfo)]

HI James, wonder how much memory saving we can get from this?
trying to decide if this is needed for Tarako. therefore 1.3? to keep in tarako radar for now
if this is not needed for tarako, will switch blocking flag back to backlog. thanks

Comment 45

[Joe Cheng [:jcheng] (please needinfo)]

Hi Dylan, is it something you can help answering? thanks

Comment 46

[Dylan Oliver [:doliver]]

I'll have to let James comment on the potential memory savings but generally I think this is a risky fix for 1.3 and therefore it's unlikely. We have tried to fix this before and had to revert it. We would need to spend more time to get this one right.

Comment 47

[James Lal [:lightsofapollo]]

The memory saved here is going to be dependent on the size of the calendar synchronized on smallish calendars the savings will be minimal (maybe none) on larger calendars (>100~) the savings will be fairly large as we will save state to idb and free memory periodically.

Comment 48

[bhavana bajaj [:bajaj]]

Joe, is the testing that's being done for calendar not acceptable enough for 1.3T? Have we done any related testing that can give some numbers here ?

Comment 49

[Brian Huang [:brianhuang][:brhuang]]

Bhavana, We did not have the test data from QA side. I will check with Joe if need to run to get the data.

Joe, if you need the data, I will ask William to provide the data in the bug.

Comment 50

[William Hsu [:whsu]]

Hi, Joe,

Could I reconfirm the requirement?
Do you mean you need the calendar loading time?

If so, I can use Buri/Taroko to do the test (Baseline).
But, I also need the patch land on master to compare the difference.
Thanks.

Comment 51

[Joe Cheng [:jcheng] (please needinfo)]

let's clarify what's needed here

it seems like with large calendars (>100), memory saving can be quite a lot and therefore, calendar app should perform better? in term of app launching / scrolling...etc (ni? James to provide further insights)

so the question for QA, how's calendar performing in terms of app launching / scrolling (unless James comes up with other comments)? do we feel current tarako calendar performance is not acceptable that we need to further improve? if so, we can probably consider this bug

Thanks

Comment 52

[James Lal [:lightsofapollo]]

>it seems like with large calendars (>100), memory saving can be quite a lot and therefore, calendar app should perform better?

The larger the calendar the bigger the savings here... this will not directly effect scrolling or launching only the time to sync the given calendars.

Comment 53

[John Hammink]

Try to get large calendar (Clint's, Ashley, Kim, etc) and sync to tarako.  Benchmark the memory, and time required.

Comment 54

[William Hsu [:whsu]]

(In reply to Joe Cheng [:jcheng] from comment #51)
> let's clarify what's needed here
> 
> it seems like with large calendars (>100), memory saving can be quite a lot
> and therefore, calendar app should perform better? in term of app launching
> / scrolling...etc (ni? James to provide further insights)
> 
> so the question for QA, how's calendar performing in terms of app launching
> / scrolling (unless James comes up with other comments)? do we feel current
> tarako calendar performance is not acceptable that we need to further
> improve? if so, we can probably consider this bug
> 
> Thanks

Hi, Joe,

Thanks for the information.
We need to prepare the test data first, then performing the related tests.

Comment 55

[Dylan Oliver [:doliver]]

Not blocking at this time until we have more definitive info from whsu's investigation. Also as stated in comment #46, this is a larger effort and we are not likely to be able to deliver this in the 1.3T time frame.

Comment 56

[John Hammink]

Clearning NI - I'll follow this if it blocks in the future.

Comment 57

[William Hsu [:whsu]]

Sorry! Other stuffs occupied my resource so I cannot reply you immediately.

Here are the detailed information

* Test parameters
  1. I only used 30~35 events to do the tests.
  2. Every event repeats 10 times on my Google calendar.
     So, my Google calendar had 300+ records.
  3. Every record contains 1000 characters at least.
  4. Syncing calendars via Wi-Fi(Mozilla Guest)

* Test cases:
  i. Cold launch time
    Step 1: Tap the calendar icon
    Step 2: Waiting until the calendar main page is displayed

 ii. Sync 30+ events
    Step 1: Tap the "Accept" button of Google authorize page
    Step 2: Waiting until first notification is popped up

iii. Cold launch time (30+ events on calendar)
    Step 1: Tap the calendar icon
    Step 2: Waiting until the calendar main page is displayed
            All events show on calendar

 ix. Remove local data (30+ events)
     Step 1: Tap "Done" button of settings page
     Step 2: Waiting until the Google account is removed from calendar

Results are as below. FYI.
- -- - -- - --- -- - -- - --- -- - -- - --
* Tarako device (V1.3T)
i. Cold launch time
  1. 1.13 sec
  2. 1.20 sec
  3. 1.15 sec
  Average: 1.16 sec
				
ii. Sync 30+ events
  1. 23.79 sec
  2. 21.21 sec
  3. 25.40 sec
  Average: 23.46 sec
				
iii. Cold launch time (30+ events on calendar)
  1. 2.52 sec
  2. 2.79 sec
  3. 2.62 sec
  Average: 2.64 sec
				
ix. Remove local data (30+ events)
  1. 7.24 sec
  2. 7.37 sec
  3. 9.07 sec
  Average: 7.89 sec
- -- - -- - --- -- - -- - --- -- - -- - --
* Buri device (V1.4)
i. Cold launch time
  1. 0.99
  2. 0.95
  3. 1.13
  Average: 1.02 sec
		
ii. Sync 30+ events
  1. 18.90 sec
  2. 18.37 sec
  3. 16.47 sec
  Average: 17.91 sec
				
iii. Cold launch time (30+ events on calendar)
  1. 2.12 sec
  2. 2.14 sec
  3. 2.50 sec
  Average: 2.25
		
ix. Remove local data (30+ events)
  1. 8.17 sec
  2. 6.01 sec
  Average: 7.09 sec
- -- - -- - --- -- - -- - --- -- - -- - --
* Build info.:
  + V1.3T with latest PAC file (0402)
   - Gaia      02b97c89dec7a10a955c85b921b2a66181eebb0e
   - Gecko     f8bca24057937f5b09d73512ba1e771011ab4203
   - BuildID   20140331101634
   - Version   28.1

  + V1.4
   - Gaia      4c3b2f57f4229c5f36f0d8fd399e65f4db88f104
   - Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/3aaca223b673
   - BuildID   20140330160202
   - Version   30.0a2

Comment 58

[BMO Automation]

Firefox OS is not being worked on