New issue
Advanced search Search tips

Issue 644026 link

Starred by 4 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 3
Type: Feature



Sign in to add a comment

Performance of Gamepad shared memory buffers can be improved

Reported by just...@gmail.com, Sep 5 2016

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.89 Safari/537.36

Steps to reproduce the problem:
1. Found via code review, steps unnecessary
2. 
3. 

What is the expected behavior?
Performance as we speed up the refresh rater of the poller remains good and doesn't cause blocking issues with the reader threads.

What went wrong?
I looked at the underlying Gamepad copy implementation and I found some issues that I’m not sure whether or not they’ll become blocking issues for WebVR frame rates.

The Gamepad is implemented as a series of providers that each query their underlying implementation. Thankfully this is on a polling thread and so it is unlikely to impact frame rates.

Then the next piece of Gamepad is the post polling copy to shared memory. From what I can tell this is more heavyweight than it should be. Copied here, you can see that while it only loops over 4 items, it is doing too much work. Specifically MapAndSanitizeGamepadData could be arbitrarily complicated and should likely occur outside of this loop. During the period of time that this loop is active, the reader thread will effectively block for up to 10 version rewrites, but this isn’t really the issue. The problem is that it blocks and yields WHILE this loop is running. I doubt that the read pass+copy would ever actually hit the 10 retry count. But the amount of time we block reading the data until the write is done is important.

  {
    base::AutoLock lock(shared_memory_lock_);

    // Acquire the SeqLock. There is only ever one writer to this data.
    // See gamepad_hardware_buffer.h.
    gamepad_shared_buffer_->WriteBegin();
    buffer->length = 0;
    for (unsigned i = 0; i < WebGamepads::itemsLengthCap; ++i) {
      PadState& state = pad_states_.get()[i];
      // Must run through the map+sanitize here or CheckForUserGesture may fail.
      MapAndSanitizeGamepadData(&state, &buffer->items[i], sanitize_);
      if (state.active_state)
        buffer->length++;
    }
    gamepad_shared_buffer_->WriteEnd();
  }

Is the easiest way to track this to open a chromium issue? And would you guys consider this interesting enough to look at? For reference here is the blocking call in the OneWriteSeqLock impl.

base::subtle::Atomic32 OneWriterSeqLock::ReadBegin() {
  base::subtle::Atomic32 version;
  for (;;) {
    version = base::subtle::NoBarrier_Load(&sequence_);

    // If the counter is even, then the associated data might be in a
    // consistent state, so we can try to read.
    if ((version & 1) == 0)
      break;

    // Otherwise, the writer is in the middle of an update. Retry the read.
    base::PlatformThread::YieldCurrentThread();
  }
  return version;
}

Here are the callers of ReadBegin(). Note how it is more likely that ReadBegin() is a problem than the following concurrency check. Also, the code should be reordered to check ReadRetry immediately following the memcpy, rather than executing a sum, cmp and branch  You can store the result of ReadRetry to a stack buffer, and then do the logic of determining if the loop should be exited.

  // Only try to read this many times before failing to avoid waiting here
  // very long in case of contention with the writer. TODO(scottmg) Tune this
  // number (as low as 1?) if histogram shows distribution as mostly
  // 0-and-maximum.
  const int kMaximumContentionCount = 10;
  int contention_count = -1;
  base::subtle::Atomic32 version;
  do {
    version = gamepad_hardware_buffer_->sequence.ReadBegin();
    memcpy(&read_into, &gamepad_hardware_buffer_->buffer, sizeof(read_into));
    ++contention_count;
    if (contention_count == kMaximumContentionCount)
      break;
  } while (gamepad_hardware_buffer_->sequence.ReadRetry(version));
  UMA_HISTOGRAM_COUNTS("Gamepad.ReadContentionCount", contention_count);

Basically, the more often we poll the writer thread, the more likely we are to hit these issues. So the more likely we are to want to optimize both the time spent in Write and the code which reads to avoid the possibility of throwing out valid data.

  // Only try to read this many times before failing to avoid waiting here
  // very long in case of contention with the writer. TODO(scottmg) Tune this
  // number (as low as 1?) if histogram shows distribution as mostly
  // 0-and-maximum.
  const int kMaximumContentionCount = 10; // Recommend following the comment and lowering?
  int contention_count = -1;
  bool shouldRetryRead = true;
  base::subtle::Atomic32 version;
  do {
    version = gamepad_hardware_buffer_->sequence.ReadBegin();
    memcpy(&read_into, &gamepad_hardware_buffer_->buffer, sizeof(read_into));
    // Ensure we read the version immediately to lower the minor chance of the writer
    // thread starting a write after we've received valid data.
    shouldRetryRead = gamepad_hardware_buffer_->sequence.ReadRetry(version);
    contention_count++;
  } while (shouldRetryRead && contention_count < kMaximumContentionCount);
  UMA_HISTOGRAM_COUNTS("Gamepad.ReadContentionCount", contention_count);

  if (shouldRetryRead) {
    return;
  }

Did this work before? No 

Chrome version: 53.0.2785.89  Channel: stable
OS Version: 10.0
Flash Version: Shockwave Flash 22.0 r0
 

Comment 1 by tkent@chromium.org, Sep 6 2016

Components: Blink>GamepadAPI
Labels: -Pri-2 Pri-3
Status: Available (was: Unconfirmed)
Project Member

Comment 3 by sheriffbot@chromium.org, Apr 11 2018

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: mattreynolds@chromium.org
Status: Available (was: Untriaged)

Sign in to add a comment