Skip to content

GitLab

T
tangled
Commit 4294b0b6 authored by Dominic Mazzoni's avatar Dominic Mazzoni Committed by Commit Bot
Browse files
Options

Avoid enqueuing extra deferred accessibility updates for the same node 

When a node's children have changed, accessibility code enqueues
information about that in order to examine the accessibility tree
after layout is clean. However, under some circumstances that led
to enqueuing the same node over and over again.

Fix that by adding a set to keep track of nodes already in the queue.

This patch also adds tracing for
AXObjectCacheImpl::ProcessDeferredAccessibilityEvents and adds a new
perf test to track this optimization. I'm seeing a ~20x speedup.

Bug: 1107988
Change-Id: I1263c5139991287a35b89c9c0e6e021f07628c17
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2340384
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: default avatarAdam Ettenberger <Adam.Ettenberger@microsoft.com>
Reviewed-by: default avatarKoji Ishii <kojii@chromium.org>
Reviewed-by: default avatarAaron Leventhal <aleventhal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#795571}
parent 68239f6a
Hide whitespace changes
Inline Side-by-side
Showing with 84 additions and 0 deletions
third_party/blink/perf_tests/accessibility/build-table.html 0 → 100644
View file @ 4294b0b6
<!DOCTYPE html>
<html>
<body>
<script src="../resources/runner.js"></script>
<table id="table"></table>
<script>
var isDone = false;
var startTime;
function runTest() {
if (startTime) {
PerfTestRunner.measureValueAsync(PerfTestRunner.now() - startTime);
PerfTestRunner.addRunTestEndMarker();
}
if (!isDone) {
PerfTestRunner.addRunTestStartMarker();
startTime = PerfTestRunner.now();
// Add 100 rows with just a header in each.
let table = document.getElementById('table');
table.innerHTML = '';
for (let i = 0; i < 100; i++) {
let row = document.createElement('tr');
row.innerHTML = '<th>Header</th>';
table.appendChild(row);
}
setTimeout(() => {
// Add 100 cells to each row.
for (let row = table.firstElementChild;
row;
row = row.nextElementSibling) {
for (let i = 0; i < 100; i++) {
let cell = document.createElement('td');
cell.innerHTML = i;
row.appendChild(cell);
}
}
}, 10);
// Wait to allow the asynchronous accessibility code that's
// covered by traceEventsToMeasure to have a chance to run.
setTimeout(runTest, 1500);
}
}
PerfTestRunner.startMeasureValuesAsync({
description: 'Test accessibility performance when dynamically building a table.',
unit: 'ms',
done: function () {
isDone = true;
},
run: function() {
runTest();
},
iterationCount: 6,
tracingCategories: 'accessibility',
traceEventsToMeasure: [
'ProcessDeferredAccessibilityEvents'
]
});
</script>
</html>
third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc
View file @ 4294b0b6
...@@ -92,6 +92,7 @@ ...@@ -92,6 +92,7 @@
#include "third_party/blink/renderer/modules/accessibility/ax_virtual_object.h" #include "third_party/blink/renderer/modules/accessibility/ax_virtual_object.h"
#include "third_party/blink/renderer/modules/media_controls/elements/media_control_elements_helper.h" #include "third_party/blink/renderer/modules/media_controls/elements/media_control_elements_helper.h"
#include "third_party/blink/renderer/modules/permissions/permission_utils.h" #include "third_party/blink/renderer/modules/permissions/permission_utils.h"
#include "third_party/blink/renderer/platform/instrumentation/tracing/trace_event.h"
#include "third_party/blink/renderer/platform/runtime_enabled_features.h" #include "third_party/blink/renderer/platform/runtime_enabled_features.h"
#include "ui/accessibility/ax_enums.mojom-blink.h" #include "ui/accessibility/ax_enums.mojom-blink.h"
#include "ui/accessibility/ax_event.h" #include "ui/accessibility/ax_event.h"
...@@ -1103,6 +1104,10 @@ void AXObjectCacheImpl::ChildrenChanged(Node* node) { ...@@ -1103,6 +1104,10 @@ void AXObjectCacheImpl::ChildrenChanged(Node* node) {
if (!node) if (!node)
return; return;
// Don't enqueue a deferred event on the same node more than once.
if (!nodes_with_pending_children_changed_.insert(node).is_new_entry)
return;
DeferTreeUpdate(&AXObjectCacheImpl::ChildrenChangedWithCleanLayout, node); DeferTreeUpdate(&AXObjectCacheImpl::ChildrenChangedWithCleanLayout, node);
} }
...@@ -1113,6 +1118,10 @@ void AXObjectCacheImpl::ChildrenChanged(const LayoutObject* layout_object) { ...@@ -1113,6 +1118,10 @@ void AXObjectCacheImpl::ChildrenChanged(const LayoutObject* layout_object) {
Node* node = GetClosestNodeForLayoutObject(layout_object); Node* node = GetClosestNodeForLayoutObject(layout_object);
if (node) { if (node) {
// Don't enqueue a deferred event on the same node more than once.
if (!nodes_with_pending_children_changed_.insert(node).is_new_entry)
return;
DeferTreeUpdate(&AXObjectCacheImpl::ChildrenChangedWithCleanLayout, node); DeferTreeUpdate(&AXObjectCacheImpl::ChildrenChangedWithCleanLayout, node);
return; return;
} }
...@@ -1170,6 +1179,8 @@ void AXObjectCacheImpl::ChildrenChangedWithCleanLayout(Node* optional_node, ...@@ -1170,6 +1179,8 @@ void AXObjectCacheImpl::ChildrenChangedWithCleanLayout(Node* optional_node,
} }
void AXObjectCacheImpl::ProcessDeferredAccessibilityEvents(Document& document) { void AXObjectCacheImpl::ProcessDeferredAccessibilityEvents(Document& document) {
TRACE_EVENT0("accessibility", "ProcessDeferredAccessibilityEvents");
if (document.Lifecycle().GetState() != DocumentLifecycle::kInAccessibility) { if (document.Lifecycle().GetState() != DocumentLifecycle::kInAccessibility) {
DCHECK(false) << "Deferred events should only be processed during the " DCHECK(false) << "Deferred events should only be processed during the "
"accessibility document lifecycle"; "accessibility document lifecycle";
...@@ -1200,6 +1211,8 @@ void AXObjectCacheImpl::ProcessUpdates(Document& document) { ...@@ -1200,6 +1211,8 @@ void AXObjectCacheImpl::ProcessUpdates(Document& document) {
TreeUpdateCallbackQueue old_tree_update_callback_queue; TreeUpdateCallbackQueue old_tree_update_callback_queue;
tree_update_callback_queue_.swap(old_tree_update_callback_queue); tree_update_callback_queue_.swap(old_tree_update_callback_queue);
nodes_with_pending_children_changed_.clear();
for (auto& tree_update : old_tree_update_callback_queue) { for (auto& tree_update : old_tree_update_callback_queue) {
Node* node = tree_update->node; Node* node = tree_update->node;
AXID axid = tree_update->axid; AXID axid = tree_update->axid;
...@@ -2290,6 +2303,7 @@ void AXObjectCacheImpl::Trace(Visitor* visitor) const { ...@@ -2290,6 +2303,7 @@ void AXObjectCacheImpl::Trace(Visitor* visitor) const {
visitor->Trace(permission_observer_receiver_); visitor->Trace(permission_observer_receiver_);
visitor->Trace(documents_); visitor->Trace(documents_);
visitor->Trace(tree_update_callback_queue_); visitor->Trace(tree_update_callback_queue_);
visitor->Trace(nodes_with_pending_children_changed_);
AXObjectCache::Trace(visitor); AXObjectCache::Trace(visitor);
} }
......
third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.h
View file @ 4294b0b6
...@@ -489,8 +489,11 @@ class MODULES_EXPORT AXObjectCacheImpl ...@@ -489,8 +489,11 @@ class MODULES_EXPORT AXObjectCacheImpl
// The main document, plus any page popups. // The main document, plus any page popups.
HeapHashSet<WeakMember<Document>> documents_; HeapHashSet<WeakMember<Document>> documents_;
// Queued callbacks.
typedef HeapVector<Member<TreeUpdateParams>> TreeUpdateCallbackQueue; typedef HeapVector<Member<TreeUpdateParams>> TreeUpdateCallbackQueue;
TreeUpdateCallbackQueue tree_update_callback_queue_; TreeUpdateCallbackQueue tree_update_callback_queue_;
HeapHashSet<WeakMember<Node>> nodes_with_pending_children_changed_;
// If tree_update_callback_queue_ gets improbably large, stop // If tree_update_callback_queue_ gets improbably large, stop
// enqueueing updates and fire a single ChildrenChanged event on the // enqueueing updates and fire a single ChildrenChanged event on the
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment