perf(robot-server): Store one command per row #14348
Conversation
SyntaxColoring
force-pushed
the
db_run_commands_as_rows
branch
from
e82a168 to
d6d0713
Compare
| def _clear_caches(self) -> None: | ||
| self.has.cache_clear() | ||
| self.get.cache_clear() | ||
| self.get_all.cache_clear() | ||
| self.get_state_summary.cache_clear() | ||
| self.get_command.cache_clear() | ||
| self._get_all_unparsed_commands.cache_clear() |
There was a problem hiding this comment.
This cache was mostly saving the computational work of repeatedly parsing the whole list of commands. It's removed here "accidentally," because we never parse the whole list of commands anymore: we only parse the slices that are requested, when they're requested.
If we wanted to retain the cache, I guess the spiritual sequel would be a pair of caches whose keys are (run_id, command_index) and (run_id, command_id). It wouldn't be as simple as an @lru_cache anymore. We could do this, I guess, but I'm skeptical that it's worthwhile. Our app no longer requests big batches of run commands. And if it did, I'd urge us to find ways to speed up the actual underlying processing, like what we did in #13425.
There was a problem hiding this comment.
I suppose for consistency, we should remove the cache on get_command(id) too.
SyntaxColoring
force-pushed
the
db_draft_migration
branch
from
f6d285c to
32f0eb1
Compare
SyntaxColoring
force-pushed
the
db_run_commands_as_rows
branch
from
0e8d735 to
98624e3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #14348 +/- ##
==========================================
- Coverage 68.29% 68.25% -0.05%
==========================================
Files 1623 2512 +889
Lines 54858 71910 +17052
Branches 4115 9174 +5059
==========================================
+ Hits 37466 49080 +11614
- Misses 16705 20664 +3959
- Partials 687 2166 +1479
Flags with carried forward coverage won't be shown. Click here to find out more.
|
SyntaxColoring
force-pushed
the
db_run_commands_as_rows
branch
from
98624e3 to
a8156e5
Compare
Overview
Closes RSS-132. See that ticket for background.
Test Plan
I think we're sufficiently covered by existing automated integration tests.
I've reviewed some of the new queries with SQLite's
EXPLAIN QUERY PLAN. They look like they're all using fast index-based searches instead of slow full table scans.Changelog
Remove the
run.commandscolumn, where each value was a large list of commands.In its place, add a
run_commandstable, where each row holds just a single command.Update
RunStoreto use the new table.Add a migration.
Review requests
This new table will be our biggest, by far. Tens of thousands of records, as opposed to ~20 in our existing tables. One of SQL's traps is that it's easy to accidentally write a very inefficient query. If the right indexes aren't set up, SQLite will degrade to a full O(n) table scan. So scrutinize my queries to make sure we're not doing that.
Also see my inline review comments.
Risk assessment
Medium. See review requests above.