Implement a ring buffer for this.log_

logs in LogStore is taken always, so adding log process must be fast. In this patch, a ring buffr is implemented. It takes O(1) time to add a log while Javascript shift() takes O(n) time. As a benchmark: 1550ms in average for writeLog 10e+7 times in prior patch. 700 ms in average for writeLog 10e+7 times in this patch. Bug: 874349 Change-Id: I49455787ecbf92583b5f63e838d7970783dc776d Reviewed-on: https://chromium-review.googlesource.com/1179432 Commit-Queue: Eriko Kurimoto <elkurin@google.com> Reviewed-by: David Tseng <dtseng@chromium.org> Reviewed-by: Yuki Awano <yawano@chromium.org> Cr-Commit-Position: refs/heads/master@{#586216}

Implement a ring buffer for this.log_
logs in LogStore is taken always, so adding log process must be fast. In this patch, a ring buffr is implemented. It takes O(1) time to add a log while Javascript shift() takes O(n) time. As a benchmark: 1550ms in average for writeLog 10e+7 times in prior patch. 700 ms in average for writeLog 10e+7 times in this patch. Bug: 874349 Change-Id: I49455787ecbf92583b5f63e838d7970783dc776d Reviewed-on: https://chromium-review.googlesource.com/1179432 Commit-Queue: Eriko Kurimoto <elkurin@google.com> Reviewed-by: David Tseng <dtseng@chromium.org> Reviewed-by: Yuki Awano <yawano@chromium.org> Cr-Commit-Position: refs/heads/master@{#586216}
c1ca8f6f · Eriko Kurimoto · Commit Bot · ab1b027e · c1ca8f6f · c1ca8f6f
Commit c1ca8f6f authored Aug 27, 2018 by Eriko Kurimoto Committed by Commit Bot Aug 27, 2018
5 changed files
--- a/chrome/browser/resources/chromeos/chromevox/BUILD.gn
+++ b/chrome/browser/resources/chromeos/chromevox/BUILD.gn
@@ -644,6 +644,7 @@ js2gtest("chromevox_extjs_tests") {
    "cvox2/background/editing_test.extjs",
    "cvox2/background/i_search_test.extjs",
    "cvox2/background/live_regions_test.extjs",
+    "cvox2/background/log_store_test.extjs",
    "cvox2/background/output_test.extjs",
    "cvox2/background/panel_test.extjs",
    "cvox2/background/recovery_strategy_test.extjs",

--- a/chrome/browser/resources/chromeos/chromevox/cvox2/background/log.html
+++ b/chrome/browser/resources/chromeos/chromevox/cvox2/background/log.html
@@ -11,6 +11,7 @@
 <body>
 <h1 class="i18n" msg="log_title">ChromeVox Log</h1>
+  <button id="clearLog">ClearLog</button>
  <label>Show: </label>
  <label>
    <input id="speechFilter" type="checkbox" class="log-filter"

--- a/chrome/browser/resources/chromeos/chromevox/cvox2/background/log.js
+++ b/chrome/browser/resources/chromeos/chromevox/cvox2/background/log.js
@@ -34,6 +34,12 @@ LogPage.init = function() {
  LogPage.backgroundWindow = chrome.extension.getBackgroundPage();
  LogPage.LogStore = LogPage.backgroundWindow.LogStore.getInstance();
+  var clearLogButton = document.getElementById('clearLog');
+  clearLogButton.onclick = function(event) {
+    LogPage.LogStore.clearLog();
+    location.reload();
+  };
  var checkboxes = document.getElementsByClassName('log-filter');
  var filterEventListener = function(event) {
    var target = event.target;
@@ -46,6 +52,10 @@ LogPage.init = function() {
  LogPage.update();
 };
+/**
+ * Update the states of checkboxes and
+ * update logs.
+ */
 LogPage.update = function() {
  for (var type in LogStore.LogType) {
    var typeFilter = LogStore.LogType[type] + 'Filter';

--- a/chrome/browser/resources/chromeos/chromevox/cvox2/background/log_store.js
+++ b/chrome/browser/resources/chromeos/chromevox/cvox2/background/log_store.js
@@ -32,17 +32,22 @@ Log = function(logStr, logType) {
 */
 LogStore = function() {
  /**
+   * Ring buffer of size this.LOG_LIMIT
   * @type {!Array<Log>}
+   * @private
   */
-  this.logs_ = [];
+  this.logs_ = Array(LogStore.LOG_LIMIT);
-  /**
+  /*
-   * @const {number}
+   * this.logs_ is implemented as a ring buffer which starts
+   * from this.startIndex_ and ends at this.startIndex_-1
+   * In the initial state, this array is filled by undefined.
+   * @type {number}
+   * @private
   */
-  this.limitLog_ = 3000;
+  this.startIndex_ = 0;
 };
 /**
 * @enum {string}
 */
@@ -54,23 +59,49 @@ LogStore.LogType = {
 };
 /**
+ * @const
+ * @private
+ */
+LogStore.LOG_LIMIT = 3000;
+/**
+ * Create logs in order.
+ * This is not the best way to create logs fast but
+ * getLogs() is not called often.
 * @return {!Array<Log>}
 */
 LogStore.prototype.getLogs = function() {
-  return this.logs_;
+  var returnLogs = [];
+  for (var i = 0; i < LogStore.LOG_LIMIT; i++) {
+    var index = (this.startIndex_ + i) % LogStore.LOG_LIMIT;
+    if (!this.logs_[index])
+      continue;
+    returnLogs.push(this.logs_[index]);
+  }
+  return returnLogs;
 };
 /**
+ * Write a log to this.logs_.
+ * To add a message to logs, this function shuold be called.
 * @param {!string} logStr
 * @param {!LogStore.LogType} logType
 */
 LogStore.prototype.writeLog = function(logStr, logType) {
  var log = new Log(logStr, logType);
-  this.logs_.push(log);
+  this.logs_[this.startIndex_] = log;
-  // TODO(elkurin): shift() takes O(n).
+  this.startIndex_ += 1;
-  // To improve performance, implement QUEUE.
+  if (this.startIndex_ == LogStore.LOG_LIMIT)
-  if (this.logs_.length > this.limitLog_)
+    this.startIndex_ = 0;
-    this.logs_.shift();
+};
+/**
+ * Clear this.logs_.
+ * Set to initial states.
+ */
+LogStore.prototype.clearLog = function() {
+  this.logs_ = Array(LogStore.LOG_LIMIT);
+  this.startIndex_ = 0;
 };
 /**
@@ -83,8 +114,7 @@ LogStore.instance;
 * @return {LogStore}
 */
 LogStore.getInstance = function() {
-  if (!LogStore.instance) {
+  if (!LogStore.instance)
    LogStore.instance = new LogStore();
-  }
  return LogStore.instance;
 };
--- a/chrome/browser/resources/chromeos/chromevox/cvox2/background/log_store_test.extjs
+++ b/chrome/browser/resources/chromeos/chromevox/cvox2/background/log_store_test.extjs
+// Copyright 2018 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.
+// Include test fixture.
+GEN_INCLUDE(['../../testing/chromevox_next_e2e_test_base.js']);
+/**
+ * Test fixture for automation_util.js.
+ * @constructor
+ * @extends {ChromeVoxE2ETestBase}
+ */
+function LogStoreTest() {
+  ChromeVoxNextE2ETest.call(this);
+}
+LogStoreTest.prototype = {
+  __proto__: ChromeVoxNextE2ETest.prototype,
+};
+SYNC_TEST_F('LogStoreTest', 'ShortLogs', function() {
+  var logStore = new LogStore();
+  for (var i = 0; i < 100; i++)
+    logStore.writeLog('test' + i, 'speech');
+  var logs = logStore.getLogs();
+  assertEquals(logs.length, 100);
+  for (var i = 0; i < logs.length; i++)
+    assertEquals(logs[i].logStr, 'test' + i);
+});
+SYNC_TEST_F('LogStoreTest', 'LongLogs', function() {
+  var logStore = new LogStore();
+  for (var i = 0; i < LogStore.LOG_LIMIT + 500; i++)
+    logStore.writeLog('test' + i, 'speech');
+  var logs = logStore.getLogs();
+  assertEquals(logs.length, LogStore.LOG_LIMIT);
+  for (var i = 0; i < logs.length; i++)
+    assertEquals(logs[i].logStr, 'test' + (i + 500));
+});