Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor loadHomeCachedLogs to use an IDB index #339

Closed
jgaehring opened this issue Apr 12, 2020 · 3 comments
Closed

Refactor loadHomeCachedLogs to use an IDB index #339

jgaehring opened this issue Apr 12, 2020 · 3 comments

Comments

@jgaehring
Copy link
Member

So I just made a first pass at the issue of loading a subset of logs for the Home screen, as first outlined in #286 (comment). It works, but it's a monster:

https://github.com/farmOS/farmOS-client/blob/9e40b756ceb8a62e8e98317cd05558bf9bdee663/src/core/store/idb/module.js#L69-L155

I don't know the O notation for this algorithm, but I'm sure it isn't good. One positive note, however, is that it shouldn't consume a lot of memory, because although it iterates through all the logs in the IDB store, it only ever holds as many as 11 of them in memory at one time. So there's that.

Talking with @mstenta though I realized a much better solution to this is to set up an IDB index, similar to an SQL index like farmOS itself uses. I could probably just use the index in place of the object store in the getRecords function:

https://github.com/farmOS/farmOS-client/blob/9e40b756ceb8a62e8e98317cd05558bf9bdee663/src/core/store/idb/idb.js#L26-L50

I could then use the index.getAll() method, which is pretty much the same as store.getAll(), but I could pass in an IDBKeyRange as the first parameter, and count as the second parameter to limit how many records are retrieved.

Ideally, with getRecords refactored to take some sort of configuration object, I could then have a loadHomeCachedLogs that looks something like this:

const loadLogs = query => openDatabase()
  .then(db => getRecords(db, logStore.name, query))
  .then((results) => {
    const cachedLogs = results.map(log => (
      updateLog(log, { isCachedLocally: true })
    ));
    commit('addLogs', cachedLogs);
  });

const lateLogsQuery = {
  // Some configuration to only retrieve undone logs with timestamps before today.
};
const upcomingLogsQuery = {
  // Some configuration to only retrieve any undone logs timestamped from
  // today to next month.
};
const doneLogsQuery = {
  // Some configuration to only retrieve the 10 most recent done logs.
};
[lateLogsQuery, upcomingLogsQuery, doneLogsQuery].forEach(loadLogs);

For that to work as desired, I might have to use some combination of index.getAll() and index.openCursor(), the latter being controlled by some sort of predicate function that would be supplied in the configuration object. We'll see.

Finally, I'm not sure if I want to do this refactoring right away, since I do at least have a method that works, albeit slowly. I think for the sake of expediency I will hold off until I've either completed the Spraying module, which is past due, or until the need arises while working on that module. I'm adding this to the 1.1 milestone for now, though it could get bumped up in priority as things progress.

@jgaehring jgaehring added this to the v1.1.0 milestone Apr 12, 2020
@tool172
Copy link

tool172 commented Apr 12, 2020 via email

@jgaehring
Copy link
Member Author

Only non completed logs are open right?

Only non-completed logs are pulled from the server, but it's possible to create completed logs locally, so both complete and incomplete logs are being stored in the local database.

I'm just starting to scan the source and api.

Awesome, thanks for taking an interest, @tool172! If you want to clone the repo and experiment on your own, be sure to check out the development documentation. If you're interested in customizing the interface, I suggest reading up on the Field Module system we're just about done implementing: #217. If you have any questions, please follow up on our forum or the farmOS Riot channel.

@jgaehring
Copy link
Member Author

I believe this is no longer necessary with the new syncing and caching API. Something like this may still prove helpful if we run into performance issues in the future with the loadCachedLogs action, but for now that seems like premature optimization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants