Don't build a dep map (twice!) when creating a new task.#757
Conversation
This moves bulk task import from O(n^2) to O(n), and new task creation from O(n) to O(1).
djmitche
left a comment
djmitche
left a comment
There was a problem hiding this comment.
I'm not convinced this is actually fixing a problem. Is the dependency_map function actually making the same DB queries more than once, or is it just called more than once? If it's making the DB queries more than once, then is something invalidating it between those fetches? The invalidation occurs in the commit_operations and commit_reversed_operations methods.
| return Ok(task); | ||
| // Check for task existence without building the depmap if it doen't exist. | ||
| if let Some(taskdata) = self.get_task_data(uuid).await? { | ||
| let depmap = self.dependency_map(false).await?; |
There was a problem hiding this comment.
The dependency_map function does nothing if the Replica already has a valid dependency map, so calling it twice isn't actually an issue.
There was a problem hiding this comment.
Ah, I missed the caching semantics, thanks!
| // map is fine and much faster. | ||
| Ok(Task::new( | ||
| TaskData::create(uuid, ops), | ||
| Arc::new(DependencyMap::new()), |
There was a problem hiding this comment.
This is incorrect, although in a relatively harmless fashion. The depmap is a map for all tasks, although it's currently only used for dependencies related to this task, of which there are none in htis case.
There was a problem hiding this comment.
Good point, I've updated the comment to better explain the change.
Yes, the invalidation is called in TaskWarriors |
2 participants
Bulk creating tasks is O(n^2) because of a redundant depmap scan.
Replica::create_taskbuilds the full depmap for a brand new task, but that map is only ever queried for the tasks own UUID and a brand new task has no dependencies or dependents.The map is invalidated on every
commit_operations, so bulk creating tasks one at a time (likeTDB2::adddoes) does a full scan on every task. This is the cause of the test timeout on GothenburgBitFactory/taskwarrior#4157This changes the new task creation to skip the scan and use an empty depmap, which will have the same results for a new task without the scan. Existing tasks still get the real depmap.