gpu: Reuse transfer buffers more aggresively

gpu/command_buffer/client/cmd_buffer_helper.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This file contains the command buffer helper class.
 
 #ifndef GPU_COMMAND_BUFFER_CLIENT_CMD_BUFFER_HELPER_H_
 #define GPU_COMMAND_BUFFER_CLIENT_CMD_BUFFER_HELPER_H_
 
 #include <string.h>
@@ skipping 14 matching lines @@
 //
 // helper.AddCommand(...);
 // helper.AddCommand(...);
 // int32 token = helper.InsertToken();
 // helper.AddCommand(...);
 // helper.AddCommand(...);
 // [...]
 //
 // helper.WaitForToken(token);  // this doesn't return until the first two
 //                              // commands have been executed.
 class GPU_EXPORT CommandBufferHelper {

[reveman, 2014/01/02 01:31:29]

What if we enumerate all internal queries and add API to this interface that
allows the mapped memory manager to handle FreePendingQuery in a way that's
consistent with how FreePendingToken currently works?

I guess we'd need:
void WaitForQuery(int32 query);
int32 last_query_completed() const;

Wdyt?

[jadahl, 2014/01/02 10:59:54]

When would we WaitForQuery? In the current implementation buffers with a query
are stored separately and not free:ed, and checked and free:ed before creating a
new buffer (in order to flush out finished buffers before getting a new one).

A query can take a very long time (especially if it was for an upload added very
late, with a long line in front of it) so it feels like that function could
stall more than what would be acceptable.

The idea you propose also makes assumption of ordering. Will such assumptions
always hold, for example if there are two uploader threads (i.e. ordering of
uploads completed won't necessarily be the same as uploads queued).

[reveman, 2014/01/02 11:56:44]

Maybe we wouldn't block on queries when trying to allocate (that might be useful
to limit memory usage though) but I think we'd need it during tear down of the
allocator.
Async upload commands are guaranteed to complete in sequential order. The
compositor is already making assumptions based on this so I think it'd OK to
make the same assumption internally as well.

[jadahl, 2014/01/03 14:13:28]

Thinking about tear down, is it really needed to wait for queries at that stage?
Since the command buffer is going down, there won't be more drawing going
through it, so even if we unmap while the uploader is still using it, it wont
cause reading free:ed memory as the service will yet to have unmapped.

In other words, with the assumptions that buffers that are still alive at the
time of destruction won't be used by anything anywhere, can't we just free and
unmap without waiting for queries?
Thinking some more about this, the idea I had was to not spread out async upload
logic outside of gles2_implementation.cc. Wouldn't it be better to limit the
number of units that need to care about queries? For example maybe limit the
exposure to gles2_implementation.cc and buffer_tracker.cc instead of adding more
state to lower levels (mapped memory, fenced allocator).

Maybe we could add enumeration to internal queries via this helper as you
proposed but have the buffer tracker manage removed-but-pending-query buffers
instead of the mapped memory manager (i.e. fenced allocator (wrapper))?

[piman, 2014/01/08 05:08:04]

I would prefer if GL concepts didn't leak into CommandBufferHelper which is
GL-agnostic.

[reveman, 2014/01/08 05:56:21]

Makes sense. What if we just call this sequence number a "async command token"
or just "async token"? As that's really what it is, a sequence number
representing a position in our async command processing pipeline. The fact that
GL queries are used by GLImplementation to determine this, does not need to be
exposed here.

  public:
   explicit CommandBufferHelper(CommandBuffer* command_buffer);
   virtual ~CommandBufferHelper();
 
   // Initializes the CommandBufferHelper.
   // Parameters:
   //   ring_buffer_size: The size of the ring buffer portion of the command
   //       buffer.
   bool Initialize(int32 ring_buffer_size);
 
@@ skipping 30 matching lines @@
 
   // Inserts a new token into the command buffer. This token either has a value
   // different from previously inserted tokens, or ensures that previously
   // inserted tokens with that value have already passed through the command
   // stream.
   // Returns:
   //   the value of the new token or -1 if the command buffer reader has
   //   shutdown.
   int32 InsertToken();
 
+  // Returns true if the token has already passed.
+  // Parameters:
+  //   the value of the token to check whether it has passed
+  bool HasTokenPassed(int32 token);

[reveman, 2014/01/02 01:31:29]

why is this needed in addition to last_token_read()?

[jadahl, 2014/01/02 10:59:54]

No need. Will remove. No idea why I added it herein the first place :P

[epennerAtGoogle, 2014/01/07 00:47:59]

Just FYI, I actually like this (as an inline function though). Even minor
reductions in logic makes code more clear and less likely to have bugs.

+
   // Waits until the token of a particular value has passed through the command
   // stream (i.e. commands inserted before that token have been executed).
   // NOTE: This will call Flush if it needs to block.
   // Parameters:
   //   the value of the token to wait for.
   void WaitForToken(int32 token);
 
   // Called prior to each command being issued. Waits for a certain amount of
   // space to be available. Returns address of space.
   CommandBufferEntry* GetSpace(uint32 entries);
@@ skipping 165 matching lines @@
   // Using C runtime instead of base because this file cannot depend on base.
   clock_t last_flush_time_;
 
   friend class CommandBufferHelperTest;
   DISALLOW_COPY_AND_ASSIGN(CommandBufferHelper);
 };
 
 }  // namespace gpu
 
 #endif  // GPU_COMMAND_BUFFER_CLIENT_CMD_BUFFER_HELPER_H_