Skip to content

Comments

feat: use double-buffer for winapi observer#1152

Merged
BoboTiG merged 2 commits intogorakhargosh:masterfrom
nascheme:winapi-double-buffer
Jan 7, 2026
Merged

feat: use double-buffer for winapi observer#1152
BoboTiG merged 2 commits intogorakhargosh:masterfrom
nascheme:winapi-double-buffer

Conversation

@nascheme
Copy link
Contributor

@nascheme nascheme commented Jan 6, 2026

This improves the performance of the winapi observer by using a second thread and by using double-buffering.

The read_directory_changes() function was replaced with a class DirectoryChangeReader() that takes care of reading the Windows handle. The ReadDirectoryChangesW() calls are made in a separate background thread and data returned is copied into a separate queue, to be processed with _parse_event_buffer() in a separate thread.

The time window for events to be lost due to processing outside the ReadDirectoryChangesW() call should be smaller. The events buffer is reused rather than creating a new one on every call.

The management of the Windows handle is encapulated in the DirectoryChangeReader() class and it takes care of waking the background thread, cleanly closing the handle and cleaning up the thread.

This partially addresses GH-1019. However, a better fix would use either overlapped IO or completion ports. However, that would be a more complex change.

This improves the performance of the winapi observer by using a second
thread and by using double-buffering.  The time window for events to be
lost due to processing outside the ReadDirectoryChangesW() call should
be smaller.  The events buffer is reused rather than creating a new one
on every call.
@BoboTiG
Copy link
Collaborator

BoboTiG commented Jan 6, 2026

Thanks @nascheme!

I guess there is at least one test to adapt on Windows, cf the failure(s) on the CI.

👍 for me when the CI will be green (take care of Windows only, we already know the Linux & macOS tests are passing).

@nascheme
Copy link
Contributor Author

nascheme commented Jan 7, 2026

Should be fixed with my latest commit (need a short sleep before ending the test__init__ case so that the background thread can cleanly exit).

@nascheme nascheme marked this pull request as ready for review January 7, 2026 04:16
@BoboTiG

This comment was marked as off-topic.

@BoboTiG BoboTiG merged commit 1e48d0b into gorakhargosh:master Jan 7, 2026
20 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants