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

Implement concurrent updates #12

Open
matteodelabre opened this issue Jan 9, 2022 · 3 comments
Open

Implement concurrent updates #12

matteodelabre opened this issue Jan 9, 2022 · 3 comments
Labels
enhancement New feature or request priority-high

Comments

@matteodelabre
Copy link
Owner

No description provided.

@matteodelabre matteodelabre added enhancement New feature or request priority-high labels Jan 9, 2022
@raisjn
Copy link

raisjn commented Jan 9, 2022

Proposal 1:

  • store frame_index for each update
  • add a new update queue called 'immediate_updates' that acts as a linked list of pending fast updates
  • in vsync thread, we iterate over immediate updates and modify the current frame by the updates inside immediate_updates and increment frame_index by one. when frame_index > len(waveform), remove that update from the immediate queue.

Problem: what if there is no regular frames sent to vsync thread but we have immediate updates in the queue?

Proposal 2:

  • store frame_index for each update
  • modify generate_frame() to optionally immediately enqueue a frame (based on frame_index) for the vsync thread instead of batch generation.
  • when running generate_frames, if the current update is immediate, we only generate one frame and increment frame_index
  • look at the next update in the queue.
    • if it is also 0) same mode, 1) immediate, 2) small region and 3) non-overlapping, we generate its frame into the current frame. repeat for next frame in queue until there are no valid updates.
    • else: we are done and go back to the top of the queue

Problem: we introduce more mutex locking by having to enqueue each frame individually (instead of batch generation)
Benefit: The subequent frame generation is likely less work because we only need to look at the region that is dirty (the full frame is already generated) and update those areas. We may not even need the "is_consecutive" check to be performed until this update is the "top" update in the queue (and needs to fill out a full frame)

@matteodelabre
Copy link
Owner Author

Thank you for this thorough proposal. Both options seem equally sensible to me. Here are some more thoughts.

For proposal 1, having no regular frames to process while there are pending immediate updates would not be an issue (in my opinion), since we can add a condition variable for the immediate queue to wake up the vsync thread when needed.

The main issue I can see in proposal 1 is that we loose the ordering between regular updates and immediate updates, since they go to different queues and are processed in different threads. Could this cause visual glitches under some circumstances? If I understand correctly, proposal 2 does not have this issue.

Another thing is that proposal 1 would blur the existing separation of concerns between the two threads, which may make it harder to reason about the code. Proposal 2 would retain this separation though, as you mentioned, this comes with the cost of more locking.

@matteodelabre
Copy link
Owner Author

matteodelabre commented Jun 17, 2022

Currently working on implementing proposal 2 on branch immediate. First results on the “spiral” test (left is with old code, right is with immediate mode):

waved-2964.mp4

Still needs more testing and cleanup before it’s ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority-high
Projects
None yet
Development

No branches or pull requests

2 participants