Skip to content
This repository has been archived by the owner on Jan 10, 2024. It is now read-only.

Poor performance due to missing caching #72

Closed
kordianbruck opened this issue Apr 5, 2017 · 12 comments · Fixed by #167
Closed

Poor performance due to missing caching #72

kordianbruck opened this issue Apr 5, 2017 · 12 comments · Fixed by #167
Assignees
Milestone

Comments

@kordianbruck
Copy link
Member

So from what I've tested the app does not seem to cache most of the data. Most noticebly it happend several times that the app was unresponsive due to load some data (10-20 seconds in the case of the calendar).

We will run into problems with the TUM. Especially the calendar uses a lot of resources on their mainframe. That should be updated once every 7 days or so at the very most! (Ask @xsrf for more details on this)

Please take a look at the Androids caching mechanics: https://github.com/TCA-Team/TumCampusApp/blob/master/app/src/main/java/de/tum/in/tumcampusapp/managers/CacheManager.java

@tgymnich
Copy link
Member

tgymnich commented Apr 16, 2017

We also need a mechanism to detect if the user enabled the token to access the calendar. And remind the user to enable it.

@nerdsupremacist
Copy link
Member

nerdsupremacist commented Apr 16, 2017

The calendar is the only thing being cached so far. Exactly for this reason. It's stored in an xml file. And updated in 7 days

@tgymnich
Copy link
Member

tgymnich commented Apr 16, 2017

@mathiasquintero If the user forgets to enable calendar access on the first app launch does he have to wait for 7 days to get a calendar?

@nerdsupremacist
Copy link
Member

nerdsupremacist commented Apr 16, 2017

Not a problem. The Manager checks the last modified date of the file. If there's no file or the date is older than 7 days: it tries to fetch the calendar again.
https://github.com/TCA-Team/iOS/blob/master/TUM%20Campus%20App/CalendarManager.swift

@tgymnich
Copy link
Member

Ok my calendar might have another issue then...

@nerdsupremacist
Copy link
Member

I'll check it anyway. Maybe the API has changed somehow

@tgymnich
Copy link
Member

tgymnich commented Apr 17, 2017

I did some performance analysis (on the main thread):

timeprofiler

Looks like the CafeteriaManager takes the longest.

timeprofiler2

@kordianbruck kordianbruck changed the title Poor performance due to caching Poor performance due to missing caching Apr 17, 2017
@kordianbruck
Copy link
Member Author

yea, we should cache all the things!

@nerdsupremacist
Copy link
Member

nerdsupremacist commented Apr 17, 2017

I personally think caching should have a lower priority. Let's finish the features. Then we can start working on a new caching layer for every manager

@tgymnich
Copy link
Member

sure

@kordianbruck
Copy link
Member Author

If you gonna spam the TumOnline API, they will turn it off for good. @xsrf right? ☠️

@tgymnich tgymnich added this to Bugfixes in iOS 1.1 Release Apr 19, 2017
@tgymnich tgymnich moved this from Bugfixes to Features in iOS 1.1 Release Apr 19, 2017
@nerdsupremacist nerdsupremacist self-assigned this Apr 21, 2017
@nerdsupremacist
Copy link
Member

I would move this to the release after 1.1. It's still High Prio. But it will require rethinking our architecture to add a caching layer without too much copy and paste

@tgymnich tgymnich removed this from Features in iOS 1.1 Release Apr 21, 2017
@kordianbruck kordianbruck added this to the 1.2 milestone Apr 21, 2017
@mammuth mammuth modified the milestones: 1.2, 1.3 Oct 18, 2017
@tgymnich tgymnich mentioned this issue Oct 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants