Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2009503003:
Mozart: Reduce pipeline depth and unify frame queue. (Closed)
DescriptionMozart: Reduce pipeline depth and unify frame queue.
Reduced default pipeline depth to 1, which is what it really
should be (if not zero!).
Entirely eliminate scheduling unnecessary draws.
Eliminated the use of a ref-counted pointers for FrameData
by retaining the current frame within the queue.
BUG=
R=mikejurka@google.com
Committed: https://chromium.googlesource.com/external/mojo/+/9b776d50db762f9c4017061042043bd30ca1a860
Patch Set 1 #
Total comments: 29
Patch Set 2 : address review comments #
Total comments: 2
Patch Set 3 : fix comments #
Messages
Total messages: 9 (2 generated)
jeffbrown@google.com changed reviewers: + mikejurka@google.com
https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... File services/gfx/compositor/backend/gpu_output.cc (right): https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:97: if (!shared_state_.frames.empty() && shared_state_.frames.front()->finished) if you introduce a pinned_for_draw variable, i would do a DCHECK here to make sure its false. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:98: shared_state_.frames.pop(); should we have a DCHECK after this pop that the front() frame is not finished? ie // Make sure all finished frames have been removed DCHECK(shared_state_.frames.empty() || !shared_state_.frames.front()->finished) https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:102: shared_state_.frames.back().swap(frame_data); if you introduce a pinned_for_draw variable, i would do a DCHECK here to make sure its false. ie DCHECK(!shared_state_.frames.back().pinned_for_draw) https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:117: ScheduleDrawLocked(); might be useful to track another time here ie draw_scheduled_time https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:197: FrameData* frame_data; probably a good idea to initialize to null (just for a safeguard for future code changes) https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:209: frame_data = shared_state_.frames.back().get(); maybe move DCHECK(!frame_data->drawn) to here instead. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:214: Do a DCHECK to ensure frame_data is not null here (just for a safeguard for future code changes) https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:218: // the frame because the |drawn| flag has been set. I would put a note in the header file next to the drawn flag that you can never remove or swap an item where (drawn && !isFinished) is true. actually, i would consider making another variable called "pinned_for_draw" that is true during this critical section (ie after "drawn" is set but before "isFinished" is set"), and check for that in the other methods. and sprinkle DCHECKs to make sure you are never popping or swapping an item with "pinned_for_draw" set to true. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:229: std::unique_ptr<FrameData> old_frame_data; // drop outside lock change comment for "drop outside lock" to "keep old frame data alive until after locked section" https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:241: frame_data->finished = true; if you decide to use a "pinned_for_draw" var, i guess you'd set it to false here as well. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:273: shared_state_.frames.front().swap(old_frame_data); I would add the earlier comment here again to clarify ie "// keep old frame data alive until after locked section" https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... File services/gfx/compositor/backend/gpu_output.h (right): https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.h:44: const int64_t submit_time; i would be a bit more descriptive, call this draw_submitted_time https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.h:46: bool finished = false; // set when drawing is finished why not call these draw_started and draw_finished? also, as mentioned in the other file, consider having a pinned_for_draw variable. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.h:47: int64_t draw_time = 0; // time when drawing began similarly, why not call this draw_start_time https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.h:48: int64_t wait_time = 0; // time when awaiting for finish began wait_time implies a timespan rather than an absolute time. what about something like rasterizer_done_time or draw_end_time (avoiding the word "finished" because you have specific semantics for that ie thats when your callback fires).
https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... File services/gfx/compositor/backend/gpu_output.cc (right): https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:97: if (!shared_state_.frames.empty() && shared_state_.frames.front()->finished) On 2016/05/25 21:11:46, mikejurka wrote: > if you introduce a pinned_for_draw variable, i would do a DCHECK here to make > sure its false. Maybe add a DCHECK here that the queue size is exactly 1. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:101: } else { Add a comment here explaining why it's safe to replace the last frame. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:117: ScheduleDrawLocked(); On 2016/05/25 21:11:46, mikejurka wrote: > might be useful to track another time here ie draw_scheduled_time I'm going to leave this out. The other two timestamps are useful for estimating overall CPU and GPU latency on their own whereas specifically observing scheduling latency is something we could accomplish through tracing. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:197: FrameData* frame_data; On 2016/05/25 21:11:46, mikejurka wrote: > probably a good idea to initialize to null (just for a safeguard for future code > changes) The compiler will yell at me if I try to use an uninitialized variable so I find it better leaving it that way rather than making the reader wonder whether a null can ever happen. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:209: frame_data = shared_state_.frames.back().get(); On 2016/05/25 21:11:46, mikejurka wrote: > maybe move DCHECK(!frame_data->drawn) to here instead. I'd rather leave it where it is since the invariant must hold true independent of whether the rasterizer is ready. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:214: On 2016/05/25 21:11:46, mikejurka wrote: > Do a DCHECK to ensure frame_data is not null here (just for a safeguard for > future code changes) I don't think that's really necessary. It must obviously be non-null since we dereferenced it. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:218: // the frame because the |drawn| flag has been set. On 2016/05/25 21:11:46, mikejurka wrote: > I would put a note in the header file next to the drawn flag that you can never > remove or swap an item where (drawn && !isFinished) is true. > > actually, i would consider making another variable called "pinned_for_draw" that > is true during this critical section (ie after "drawn" is set but before > "isFinished" is set"), and check for that in the other methods. and sprinkle > DCHECKs to make sure you are never popping or swapping an item with > "pinned_for_draw" set to true. I don't actually need extra state here. I could add an inline function but I think the indirection may make the code even harder to understand. Note that this invariant follows from the comments already in the header file. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:229: std::unique_ptr<FrameData> old_frame_data; // drop outside lock On 2016/05/25 21:11:46, mikejurka wrote: > change comment for "drop outside lock" to "keep old frame data alive until after > locked section" Done. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:273: shared_state_.frames.front().swap(old_frame_data); On 2016/05/25 21:11:46, mikejurka wrote: > I would add the earlier comment here again to clarify ie > > "// keep old frame data alive until after locked section" Done. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... File services/gfx/compositor/backend/gpu_output.h (right): https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.h:44: const int64_t submit_time; On 2016/05/25 21:11:46, mikejurka wrote: > i would be a bit more descriptive, call this draw_submitted_time I think that might get confused with draw_time. submit_time is actually set in SubmitFrame which I think makes sense. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.h:46: bool finished = false; // set when drawing is finished On 2016/05/25 21:11:46, mikejurka wrote: > why not call these draw_started and draw_finished? > > also, as mentioned in the other file, consider having a pinned_for_draw > variable. Sure, draw_started and draw_finished makes sense. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.h:47: int64_t draw_time = 0; // time when drawing began On 2016/05/25 21:11:46, mikejurka wrote: > similarly, why not call this draw_start_time Ok. https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.h:48: int64_t wait_time = 0; // time when awaiting for finish began On 2016/05/25 21:11:46, mikejurka wrote: > wait_time implies a timespan rather than an absolute time. what about something > like rasterizer_done_time or draw_end_time (avoiding the word "finished" because > you have specific semantics for that ie thats when your callback fires). draw_issued_time?
just commented on two places where you said 'Done' but i think the change didn't make it in the patchset. otherwise, looks great!! much clearer. lgtm :) https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... File services/gfx/compositor/backend/gpu_output.cc (right): https://codereview.chromium.org/2009503003/diff/1/services/gfx/compositor/bac... services/gfx/compositor/backend/gpu_output.cc:98: shared_state_.frames.pop(); actually, on second thought, shouldn't the above "if" clause be completely unnecessary? doesn't OnRasterizerFinishedDraw set "finished" to true and then remove from the queue in one swoop? https://codereview.chromium.org/2009503003/diff/20001/services/gfx/compositor... File services/gfx/compositor/backend/gpu_output.cc (right): https://codereview.chromium.org/2009503003/diff/20001/services/gfx/compositor... services/gfx/compositor/backend/gpu_output.cc:241: std::unique_ptr<FrameData> old_frame_data; // drop outside lock change comment for "drop outside lock" to "keep old frame data alive until after locked section" https://codereview.chromium.org/2009503003/diff/20001/services/gfx/compositor... services/gfx/compositor/backend/gpu_output.cc:286: shared_state_.frames.front().swap(old_frame_data); I would add the earlier comment here again to clarify ie "// keep old frame data alive until after locked section"
please ignore the comment on gpu_output.cc:98, that was from before and meant to delete it.
please ignore the comment on gpu_output.cc:98, that was from before and meant to delete it.
Description was changed from ========== Mozart: Reduce pipeline depth and unify frame queue. Reduced default pipeline depth to 1, which is what it really should be (if not zero!). Entirely eliminate scheduling unnecessary draws. Eliminated the use of a ref-counted pointers for FrameData by retaining the current frame within the queue. BUG= ========== to ========== Mozart: Reduce pipeline depth and unify frame queue. Reduced default pipeline depth to 1, which is what it really should be (if not zero!). Entirely eliminate scheduling unnecessary draws. Eliminated the use of a ref-counted pointers for FrameData by retaining the current frame within the queue. BUG= R=mikejurka@google.com Committed: https://chromium.googlesource.com/external/mojo/+/9b776d50db762f9c40170610420... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 9b776d50db762f9c4017061042043bd30ca1a860 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
