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

Read multiple bytes for system events #10

Merged
merged 6 commits into from
Nov 6, 2024

Conversation

tlsomers
Copy link

When checking for system events, the current version would read 1 byte per iteration, meaning that when other tasks are ready, reading n bytes becomes interleaved with n other tasks. These tasks may be slow, resulting in unnecessary slowdowns of system events (such as UI events in vscoq).

This change allows reading another byte from its file descriptor when:

  1. The system event has lower priority than the lowest priority ready event
  2. The event just read an available byte from the file descriptor.

@gares
Copy link
Owner

gares commented Nov 18, 2023

Thanks for your contribution, I'll try to double check the code again on Monday, but it looks OK to me.
One thing I'd like to have is a test. I think you can verify the invariant by using 2 pipes and two events of different priorities.
You can then check the second pipe is still full after sel returns with the first event being ready.
Some benchmarking would also be nice, but I don't have a proper infrastructure.

There is another approach that may be worth considering:

  • read more data here
    let n = Unix.read fd buff 0 1 in
  • pass the extra data (after the \n) to the following event:

    sel/lib/events.ml

    Lines 166 to 170 in bccdff5

    one_line ()
    --? (parse_content_length_or (finish_with k) (fun length ->
    one_line ()
    --? (fun _discard ->
    some_bytes length ()

I suspect that VSCode sends the whole header in one go (and maybe also the payload). In this way the number of iterations of the loop pulling ready events is going to decrease substantially.

@gares
Copy link
Owner

gares commented Nov 6, 2024

I took me a while, but here the result:

  • I now distinguish between events that are ready, that did make some progress and that made no progress
  • If an event makes progress (or is over) we give another chance to events of the same or lower priority that did advance
  • We compare priorities using the user part, since internally priorities are a pair (user,timestamp) and for the reasoning above we really don't care about the timestamp (that is used to preserve insertion order)

Both tests fail in main, and work in this branch.

I think that the behavior is now: when an system event starts to make progress, we guarantee we give it enough rounds to complete if his priority is the lowest among the events that are ready or made progress in the previous round. In particular a system even with lower priority than a ready task (Sel.now) does complete (if there is enough data to read).

@gares gares merged commit a5ec48e into gares:main Nov 6, 2024
3 checks passed
@gares gares mentioned this pull request Nov 6, 2024
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