perf(file-system): Fix quadratic file rescans in the FS request queue#2011
perf(file-system): Fix quadratic file rescans in the FS request queue#2011vdusek wants to merge 1 commit into
Conversation
BREAKING CHANGE: The file system request queue no longer emits the 'Request <unique_key> not found in state, skipping.' warning. The cache refresh no longer scans the queue directory, so orphaned request files (e.g. written by another process) are silently ignored.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2011 +/- ##
==========================================
+ Coverage 93.35% 93.44% +0.09%
==========================================
Files 179 179
Lines 12482 12467 -15
==========================================
- Hits 11652 11650 -2
+ Misses 830 817 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Mantisus
left a comment
There was a problem hiding this comment.
LGTM. Only few suggestions.
| if request is not None: | ||
| self._request_cache.append(request) |
There was a problem hiding this comment.
Let's add a log warning when there is no file for the unique_key.
| if len(self._request_cache) < self._MAX_REQUESTS_IN_CACHE: | ||
| self._request_cache.appendleft(request) |
There was a problem hiding this comment.
We can probably skip this if self._request_cache_needs_refresh is already True.
| async def test_forefront_add_fetch_handle_parses_linear_number_of_files( | ||
| rq_client: FileSystemRequestQueueClient, monkeypatch: pytest.MonkeyPatch | ||
| ) -> None: | ||
| """Test that per-request forefront adds (the `RequestManagerTandem` pattern) do not rescan all request files.""" | ||
| parse_count = 0 | ||
| original_parse = type(rq_client)._parse_request_file | ||
|
|
||
| async def counting_parse(file_path: Path) -> Request | None: | ||
| nonlocal parse_count | ||
| parse_count += 1 | ||
| return await original_parse(file_path) | ||
|
|
||
| monkeypatch.setattr(type(rq_client), '_parse_request_file', staticmethod(counting_parse)) | ||
|
|
||
| n = 25 | ||
| for i in range(n): | ||
| await rq_client.add_batch_of_requests([Request.from_url(f'https://example.com/{i}')], forefront=True) | ||
| request = await rq_client.fetch_next_request() | ||
| assert request is not None | ||
| await rq_client.mark_request_as_handled(request) | ||
|
|
||
| # Before the fix, each forefront add invalidated the cache and each fetch re-parsed every request file | ||
| # ever written, giving n * (n + 1) / 2 parses in total. With the fix, new forefront requests go straight | ||
| # to the cache, so only the initial refresh parses anything. | ||
| assert parse_count <= 5 |
There was a problem hiding this comment.
Maybe we could use unittest.mock.patch for this?
For example
from unittest.mock import patch
async def test_forefront_add_fetch_handle_parses_linear_number_of_files(
rq_client: FileSystemRequestQueueClient,
) -> None:
with patch.object(
rq_client,
'_parse_request_file',
wraps=rq_client._parse_request_file,
) as call_counter:
n = 25
for i in range(n):
await rq_client.add_batch_of_requests([Request.from_url(f'https://example.com/{i}')], forefront=True)
request = await rq_client.fetch_next_request()
assert request is not None
await rq_client.mark_request_as_handled(request)
assert call_counter.call_count <= 5
Problem
The file system request queue client was O(n²):
forefront=Trueinvalidated the whole cache, andRequestManagerTandemadds every loader request withforefront=True, which triggered a full rescan per processed request.json.loadran on the event loop.Processing a 10k
RequestListthrough the tandem meant roughly 50M file parses.Changes
_refresh_cachenow derives pending requests from the recoverable state's key→sequence maps and reads only those files, instead of globbing and parsing the whole directory.handled_requestsfor deduplication), so cache refreshes,is_empty, and the persisted state blob scale with the backlog rather than with every request ever processed._parse_request_filereads the file in a singleasyncio.to_threadcall instead of runningjson.loadon the event loop.Results
Processing 200 requests through the tandem pattern went from 20,100 file parses (2.8 s) to 1 (0.3 s), and scaling is now linear (2,000 requests in about 2.6 s). Ordering semantics (forefront LIFO, regular FIFO), deduplication, and crash-recovery behavior are preserved and covered by new regression tests.
Logging changes
The file system request queue no longer emits the
Request <unique_key> not found in state, skipping.warning. The cache refresh no longer scans the queue directory, so orphaned request files (e.g. written by another process) are silently ignored.