Show how far accept all has got - #874
Merged
Merged
Conversation
Accept all takes as long as the queue is long - every snapshot is a
read, a patch and a rewrite of its source under a cross process mutex,
and a move can be retried for seconds while a diff tool lets go of it -
and nothing said so. A viewer that owned the queue ran the whole batch
as one transition on its render thread, so the window stopped painting
until it was done and then emptied at once. A viewer displaying the
tray's queue said "Waiting for the queue owner." over a list that did
not move: the tray applied everything before recording anything, and
the viewer only listed once its accept-all came back. A batch longer
than fifteen seconds then reported the owner as no longer running while
it was still accepting.
The viewer now carries out an accept-all an entry at a time.
BeginAcceptAll records an AcceptBatch, and AcceptAllRunner claims an
entry under the session's lock, applies it outside, and records it
under the lock again, so between entries the window draws the queue
shrinking and the status line says "Accepting 12 of 40". A batch
started in the window runs on a worker, one the tray asked for runs on
the listener thread, and only one drives at a time. Snapshots go before
files, since whether a delete is held turns on how the snapshots went.
ViewerSession.Apply(AcceptAll) is the same steps back to back, which is
what the existing tests already drive. While a batch runs, Accept,
Discard and Accept all are disabled and their keys and menu items do
nothing; scrolling, selecting, folding and copying carry on.
The tray records each snapshot as it lands, with
InlineQueue.AcceptInBatch - the batch's rules, one entry at a time,
which AcceptAll over a list of outcomes is now made of - and its
listings carry how far the batch has got, the tracked files included.
The listing gained an optional "progress: done|total" line, which a
reader that predates it skips. OwnerLink lists beside a forwarded
command rather than after it, so a window attached to the tray follows
the batch whichever surface started it, and a finished command's
message still arrives with the listing that shows its effect. Forwarded
commands, and the tray's accept-all against a viewer, now wait five
minutes rather than fifteen seconds, since the listing is what says
whether the owner is still there.
Checked against the real Windows viewer, 40 queued snapshots with one
file's patch mutex held for five seconds: the window showed "Accepting
6 of 40" over the 35 still pending, with the acting buttons disabled,
and its listing answered throughout; all 40 landed once the mutex was
let go. On main the same run showed "Pending (40)" and "lines 1-1 of 1"
with five files already written, and the listing did not answer until
the mutex was let go.
Group accepts ("Accept all in ...") are still one transition; they
drop stale entries where accept-all keeps them, so moving them onto the
batch is its own change.
ViewerLaunchTests.AcceptAllOverALongQueue opens the viewer on 503 pending snapshots for a person to press Accept all on: two solutions of ten classes, twenty five multi line snapshots a class, so every accept moves the call sites below it, plus three conflicts that keep the window open afterwards to show what the batch said. A fast disk accepts all of it in a second or two, too quick to watch, so the test holds the class halfway down the batch by the mutex InlineApplier waits on, the way another process writing it would. The count stops there for six seconds while the window keeps answering, then carries on. It is taken only once a batch has started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Accept all takes as long as the queue is long - every snapshot is a read, a patch and a rewrite of its source under a cross process mutex, and a move can be retried for seconds while a diff tool lets go of it - and nothing said so. A viewer that owned the queue ran the whole batch as one transition on its render thread, so the window stopped painting until it was done and then emptied at once. A viewer displaying the tray's queue said "Waiting for the queue owner." over a list that did not move: the tray applied everything before recording anything, and the viewer only listed once its accept-all came back. A batch longer than fifteen seconds then reported the owner as no longer running while it was still accepting.
The viewer now carries out an accept-all an entry at a time.
BeginAcceptAllrecords anAcceptBatch, andAcceptAllRunnerclaims an entry under the session's lock, applies it outside, and records it under the lock again, so between entries the window draws the queue shrinking and the status line says "Accepting 12 of 40". A batch started in the window runs on a worker, one the tray asked for runs on the listener thread, and only one drives at a time. Snapshots go before files, since whether a delete is held turns on how the snapshots went.ViewerSession.Apply(AcceptAll)is the same steps back to back, which is what the existing tests already drive. While a batch runs, Accept, Discard and Accept all are disabled and their keys and menu items do nothing; scrolling, selecting, folding and copying carry on.The tray records each snapshot as it lands, with
InlineQueue.AcceptInBatch- the batch's rules, one entry at a time, whichAcceptAllover a list of outcomes is now made of - and its listings carry how far the batch has got, the tracked files included. The listing gained an optionalprogress: done|totalline, which a reader that predates it skips.OwnerLinklists beside a forwarded command rather than after it, so a window attached to the tray follows the batch whichever surface started it, and a finished command's message still arrives with the listing that shows its effect. Forwarded commands, and the tray's accept-all against a viewer, now wait five minutes rather than fifteen seconds, since the listing is what says whether the owner is still there.Checked against the real Windows viewer, 40 queued snapshots with one file's patch mutex held for five seconds: the window showed "Accepting 6 of 40" over the 35 still pending, with the acting buttons disabled, and its listing answered throughout; all 40 landed once the mutex was let go. On main the same run showed "Pending (40)" and "lines 1-1 of 1" with five files already written, and the listing did not answer until the mutex was let go.
AcceptAllProgressTestssteps a batch and holds one apply part way through, over the session, the runner, the wire and an attached viewer's polling loop;InlineScreenTests.AcceptAllInProgressis the screen part way through;TrayViewerSyncTest.AViewerAttachedToTheTrayFollowsItsAcceptAllis the arrangement the tray sets up at login, end to end over a real socket.Group accepts ("Accept all in ...") are still one transition. They drop stale entries where accept-all keeps them, so moving them onto the batch is its own change.