Commit 12a5ac69 authored by Kevin Babbitt's avatar Kevin Babbitt Committed by Commit Bot

Optimize AXPosition CompareTo using uncommon ancestors

Testing with Windows Narrator scan mode on Wikipedia showed we were
spending an inordinate amount of time comparing text positions due to
the cost of creating ancestor positions. This CL avoids that cost by
comparing the order of the first uncommon ancestors for each node, when
it's possible to do so.

Testing on https://en.wikipedia.org/wiki/List_of_highest-grossing_films
with Narrator scan mode heading movement showed a reduction of about 87%
CPU usage on the browser main thread with this CL.

Bug: 1029867
Change-Id: I7efae50517a561319dad81668c49ec5f6ae900b5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1999083
Commit-Queue: Kevin Babbitt <kbabbitt@microsoft.com>
Reviewed-by: default avatarDominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: default avatarJacques Newman <janewman@microsoft.com>
Reviewed-by: default avatarAdam Ettenberger <adettenb@microsoft.com>
Reviewed-by: default avatarEthan Jimenez <ethavar@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#732522}
parent 68ef8178
...@@ -138,6 +138,16 @@ BrowserAccessibility* BrowserAccessibilityPosition::GetNodeInTree( ...@@ -138,6 +138,16 @@ BrowserAccessibility* BrowserAccessibilityPosition::GetNodeInTree(
return manager->GetFromID(node_id); return manager->GetFromID(node_id);
} }
int32_t BrowserAccessibilityPosition::GetAnchorID(
BrowserAccessibility* node) const {
return node->GetId();
}
AXTreeID BrowserAccessibilityPosition::GetTreeID(
BrowserAccessibility* node) const {
return node->manager()->ax_tree_id();
}
bool BrowserAccessibilityPosition::IsEmbeddedObjectInParent() const { bool BrowserAccessibilityPosition::IsEmbeddedObjectInParent() const {
// On some platforms, most objects are represented in the text of their // On some platforms, most objects are represented in the text of their
// parents with a special (embedded object) character and not with their // parents with a special (embedded object) character and not with their
......
...@@ -47,6 +47,9 @@ class CONTENT_EXPORT BrowserAccessibilityPosition ...@@ -47,6 +47,9 @@ class CONTENT_EXPORT BrowserAccessibilityPosition
ui::AXNode::AXID* parent_id) const override; ui::AXNode::AXID* parent_id) const override;
BrowserAccessibility* GetNodeInTree(AXTreeID tree_id, BrowserAccessibility* GetNodeInTree(AXTreeID tree_id,
ui::AXNode::AXID node_id) const override; ui::AXNode::AXID node_id) const override;
int32_t GetAnchorID(BrowserAccessibility* node) const override;
AXTreeID GetTreeID(BrowserAccessibility* node) const override;
bool IsEmbeddedObjectInParent() const override; bool IsEmbeddedObjectInParent() const override;
bool IsInLineBreakingObject() const override; bool IsInLineBreakingObject() const override;
......
...@@ -153,6 +153,14 @@ AXNode* AXNodePosition::GetNodeInTree(AXTreeID tree_id, ...@@ -153,6 +153,14 @@ AXNode* AXNodePosition::GetNodeInTree(AXTreeID tree_id,
return nullptr; return nullptr;
} }
int32_t AXNodePosition::GetAnchorID(AXNode* node) const {
return node->id();
}
AXTreeID AXNodePosition::GetTreeID(AXNode* node) const {
return node->tree()->GetAXTreeID();
}
base::string16 AXNodePosition::GetText() const { base::string16 AXNodePosition::GetText() const {
if (IsNullPosition()) if (IsNullPosition())
return {}; return {};
......
...@@ -52,6 +52,8 @@ class AX_EXPORT AXNodePosition : public AXPosition<AXNodePosition, AXNode> { ...@@ -52,6 +52,8 @@ class AX_EXPORT AXNodePosition : public AXPosition<AXNodePosition, AXNode> {
base::stack<AXNode*> GetAncestorAnchors() const override; base::stack<AXNode*> GetAncestorAnchors() const override;
void AnchorParent(AXTreeID* tree_id, AXNode::AXID* parent_id) const override; void AnchorParent(AXTreeID* tree_id, AXNode::AXID* parent_id) const override;
AXNode* GetNodeInTree(AXTreeID tree_id, AXNode::AXID node_id) const override; AXNode* GetNodeInTree(AXTreeID tree_id, AXNode::AXID node_id) const override;
int32_t GetAnchorID(AXNode* node) const override;
AXTreeID GetTreeID(AXNode* node) const override;
bool IsInLineBreakingObject() const override; bool IsInLineBreakingObject() const override;
ax::mojom::Role GetRole() const override; ax::mojom::Role GetRole() const override;
......
...@@ -168,4 +168,23 @@ TEST_F(AXPositionPerfTest, AsLeafTextPositionFromTreePosition) { ...@@ -168,4 +168,23 @@ TEST_F(AXPositionPerfTest, AsLeafTextPositionFromTreePosition) {
reporter.AddResult(kMetricCallsPerSecondRunsPerS, timer.LapsPerSecond()); reporter.AddResult(kMetricCallsPerSecondRunsPerS, timer.LapsPerSecond());
} }
TEST_F(AXPositionPerfTest, CompareTextPositions) {
TestPositionType text_position_1 = AXNodePosition::CreateTextPosition(
tree_.data().tree_id, /*anchor_id=*/7, /*text_offset=*/1,
ax::mojom::TextAffinity::kDownstream);
TestPositionType text_position_2 = AXNodePosition::CreateTextPosition(
tree_.data().tree_id, /*anchor_id=*/27, /*text_offset=*/1,
ax::mojom::TextAffinity::kDownstream);
base::LapTimer timer(kWarmupLaps, base::TimeDelta(), kLaps);
for (int i = 0; i < kLaps + kWarmupLaps; ++i) {
text_position_1->CompareTo(*text_position_2);
timer.NextLap();
}
auto reporter = SetUpReporter("CompareTextPositions");
reporter.AddResult(kMetricCallsPerSecondRunsPerS, timer.LapsPerSecond());
}
} // namespace ui } // namespace ui
...@@ -871,6 +871,13 @@ TEST_F(AXPositionTest, IsIgnored) { ...@@ -871,6 +871,13 @@ TEST_F(AXPositionTest, IsIgnored) {
new_tree->data().tree_id, root_data.id, 2 /* child_index */); new_tree->data().tree_id, root_data.id, 2 /* child_index */);
ASSERT_TRUE(tree_position_5->IsTreePosition()); ASSERT_TRUE(tree_position_5->IsTreePosition());
EXPECT_FALSE(tree_position_5->IsIgnored()); EXPECT_FALSE(tree_position_5->IsIgnored());
// A "before text" position on an unignored node should not be ignored.
TestPositionType tree_position_6 = AXNodePosition::CreateTreePosition(
new_tree->data().tree_id, static_text_data_1.id,
AXNodePosition::BEFORE_TEXT);
ASSERT_TRUE(tree_position_6->IsTreePosition());
EXPECT_FALSE(tree_position_6->IsIgnored());
} }
TEST_F(AXPositionTest, GetTextFromNullPosition) { TEST_F(AXPositionTest, GetTextFromNullPosition) {
...@@ -5606,6 +5613,40 @@ TEST_F(AXPositionTest, OperatorEquals) { ...@@ -5606,6 +5613,40 @@ TEST_F(AXPositionTest, OperatorEquals) {
ASSERT_NE(nullptr, text_position2); ASSERT_NE(nullptr, text_position2);
ASSERT_TRUE(text_position2->IsTextPosition()); ASSERT_TRUE(text_position2->IsTextPosition());
EXPECT_EQ(*text_position1, *text_position2); EXPECT_EQ(*text_position1, *text_position2);
// Two "after text" positions on a parent and child should be equivalent, in
// the middle of the document...
text_position1 = AXNodePosition::CreateTextPosition(
tree_.data().tree_id, static_text1_.id, 6 /* text_offset */,
ax::mojom::TextAffinity::kDownstream);
ASSERT_NE(nullptr, text_position1);
ASSERT_TRUE(text_position1->IsTextPosition());
text_position2 = AXNodePosition::CreateTextPosition(
tree_.data().tree_id, inline_box1_.id, 6 /* text_offset */,
ax::mojom::TextAffinity::kDownstream);
ASSERT_NE(nullptr, text_position2);
ASSERT_TRUE(text_position2->IsTextPosition());
EXPECT_EQ(*text_position1, *text_position2);
// ...and at the end of the document.
text_position1 = AXNodePosition::CreateTextPosition(
tree_.data().tree_id, static_text2_.id, 6 /* text_offset */,
ax::mojom::TextAffinity::kDownstream);
ASSERT_NE(nullptr, text_position1);
ASSERT_TRUE(text_position1->IsTextPosition());
text_position2 = AXNodePosition::CreateTextPosition(
tree_.data().tree_id, inline_box2_.id, 6 /* text_offset */,
ax::mojom::TextAffinity::kDownstream);
ASSERT_NE(nullptr, text_position2);
ASSERT_TRUE(text_position2->IsTextPosition());
// Validate that we're actually at the end of the document by normalizing to
// the equivalent "before character" position.
EXPECT_TRUE(
text_position1->AsLeafTextPositionBeforeCharacter()->IsNullPosition());
EXPECT_TRUE(
text_position2->AsLeafTextPositionBeforeCharacter()->IsNullPosition());
// Now compare the positions.
EXPECT_EQ(*text_position1, *text_position2);
} }
TEST_F(AXPositionTest, OperatorEqualsSameTextOffsetSameAnchorId) { TEST_F(AXPositionTest, OperatorEqualsSameTextOffsetSameAnchorId) {
...@@ -5770,6 +5811,25 @@ TEST_F(AXPositionTest, OperatorsLessThanAndGreaterThan) { ...@@ -5770,6 +5811,25 @@ TEST_F(AXPositionTest, OperatorsLessThanAndGreaterThan) {
ASSERT_NE(nullptr, text_position2); ASSERT_NE(nullptr, text_position2);
ASSERT_TRUE(text_position2->IsTextPosition()); ASSERT_TRUE(text_position2->IsTextPosition());
EXPECT_EQ(*text_position1, *text_position2); EXPECT_EQ(*text_position1, *text_position2);
// A text position at the end of the document versus one that isn't.
text_position1 = AXNodePosition::CreateTextPosition(
tree_.data().tree_id, inline_box2_.id, 6 /* text_offset */,
ax::mojom::TextAffinity::kDownstream);
ASSERT_NE(nullptr, text_position1);
ASSERT_TRUE(text_position1->IsTextPosition());
// Validate that we're actually at the end of the document by normalizing to
// the equivalent "before character" position.
EXPECT_TRUE(
text_position1->AsLeafTextPositionBeforeCharacter()->IsNullPosition());
// Now create the not-at-end-of-document position and compare.
text_position2 = AXNodePosition::CreateTextPosition(
tree_.data().tree_id, static_text2_.id, 0 /* text_offset */,
ax::mojom::TextAffinity::kDownstream);
ASSERT_NE(nullptr, text_position2);
ASSERT_TRUE(text_position2->IsTextPosition());
EXPECT_GT(*text_position1, *text_position2);
EXPECT_LT(*text_position2, *text_position1);
} }
TEST_F(AXPositionTest, Swap) { TEST_F(AXPositionTest, Swap) {
......
...@@ -340,6 +340,10 @@ class AXPosition { ...@@ -340,6 +340,10 @@ class AXPosition {
NOTREACHED(); NOTREACHED();
return false; return false;
case AXPositionKind::TREE_POSITION: { case AXPositionKind::TREE_POSITION: {
// If this is a "before text" position, it's pointing to the anchor
// itself, which we've determined to be unignored.
if (child_index_ == BEFORE_TEXT)
return false;
// If there is a node at the position pointed to by "child_index_", i.e. // If there is a node at the position pointed to by "child_index_", i.e.
// this position is neither a leaf position nor an "after children" // this position is neither a leaf position nor an "after children"
// position, consider this tree position to be ignored if the child node // position, consider this tree position to be ignored if the child node
...@@ -2544,6 +2548,104 @@ class AXPosition { ...@@ -2544,6 +2548,104 @@ class AXPosition {
if (this->IsNullPosition() || other.IsNullPosition()) if (this->IsNullPosition() || other.IsNullPosition())
return base::Optional<int>(base::nullopt); return base::Optional<int>(base::nullopt);
// If both positions share an anchor and are of the same type, we can do a
// straight compare of text offsets or child indices.
if (GetAnchor() == other.GetAnchor()) {
if (IsTextPosition() && other.IsTextPosition())
return text_offset() - other.text_offset();
if (IsTreePosition() && other.IsTreePosition())
return child_index() - other.child_index();
}
// Ancestor positions are expensive to compute. If possible, we will avoid
// doing so by computing the ancestor chain of the two positions' anchors.
// If the lowest common ancestor is neither position's anchor, we can use
// the order of the first uncommon ancestors as a proxy for the order of the
// positions.
//
// In order to do that, we need to normalize text positions at the end of an
// anchor to equivalent positions at the start of the next anchor. Ignored
// positions are a special case in that they need to be shifted to the
// nearest unignored position in order to be normalized. That shifting can
// change the comparison result, so if we have an ignored position, we must
// use the slow path.
if (IsIgnored() || other.IsIgnored())
return SlowCompareTo(other);
// Normalize any text positions at the end of an anchor to equivalent
// positions at the start of the next anchor.
AXPositionInstance normalized_this_position = Clone();
if (normalized_this_position->IsTextPosition())
normalized_this_position =
normalized_this_position->AsLeafTextPositionBeforeCharacter();
AXPositionInstance normalized_other_position = other.Clone();
if (normalized_other_position->IsTextPosition())
normalized_other_position =
normalized_other_position->AsLeafTextPositionBeforeCharacter();
if (normalized_this_position->IsNullPosition()) {
if (normalized_other_position->IsNullPosition()) {
// Both positions normalized to a position past the end of the document.
DCHECK_EQ(SlowCompareTo(other).value(), 0);
return 0;
}
// |this| normalized to a position past the end of the document.
DCHECK_GT(SlowCompareTo(other).value(), 0);
return 1;
} else if (normalized_other_position->IsNullPosition()) {
// |other| normalized to a position past the end of the document.
DCHECK_LT(SlowCompareTo(other).value(), 0);
return -1;
}
// Compute the ancestor stacks of both positions and walk them ourselves
// rather than calling LowestCommonAnchor(). That way, we can discover the
// first uncommon ancestors.
const AXNodeType* common_anchor = nullptr;
base::stack<AXNodeType*> our_ancestors =
normalized_this_position->GetAncestorAnchors();
base::stack<AXNodeType*> other_ancestors =
normalized_other_position->GetAncestorAnchors();
while (!our_ancestors.empty() && !other_ancestors.empty() &&
our_ancestors.top() == other_ancestors.top()) {
common_anchor = our_ancestors.top();
our_ancestors.pop();
other_ancestors.pop();
}
if (!common_anchor)
return base::Optional<int>(base::nullopt);
// If each position has an uncommon ancestor node, we can compare those
// instead of needing to compute ancestor positions.
if (!our_ancestors.empty() && !other_ancestors.empty()) {
int this_uncommon_ancestor_index =
CreateTreePosition(GetTreeID(our_ancestors.top()),
GetAnchorID(our_ancestors.top()),
0 /*child_index*/)
->AnchorIndexInParent();
int other_uncommon_ancestor_index =
CreateTreePosition(GetTreeID(other_ancestors.top()),
GetAnchorID(other_ancestors.top()),
0 /*child_index*/)
->AnchorIndexInParent();
DCHECK(this_uncommon_ancestor_index != other_uncommon_ancestor_index);
int result = this_uncommon_ancestor_index - other_uncommon_ancestor_index;
#if DCHECK_IS_ON()
// Validate the optimization.
int slow_result = SlowCompareTo(other).value();
DCHECK((result < 0 && slow_result < 0) ||
(result > 0 && slow_result > 0));
#endif
return result;
}
return SlowCompareTo(other);
}
base::Optional<int> SlowCompareTo(const AXPosition& other) const {
// It is potentially costly to compute the parent position of a text // It is potentially costly to compute the parent position of a text
// position, whilst computing the parent position of a tree position is // position, whilst computing the parent position of a tree position is
// really inexpensive. In order to find the lowest common ancestor, // really inexpensive. In order to find the lowest common ancestor,
...@@ -2788,6 +2890,8 @@ class AXPosition { ...@@ -2788,6 +2890,8 @@ class AXPosition {
virtual void AnchorParent(AXTreeID* tree_id, int32_t* parent_id) const = 0; virtual void AnchorParent(AXTreeID* tree_id, int32_t* parent_id) const = 0;
virtual AXNodeType* GetNodeInTree(AXTreeID tree_id, virtual AXNodeType* GetNodeInTree(AXTreeID tree_id,
int32_t node_id) const = 0; int32_t node_id) const = 0;
virtual int32_t GetAnchorID(AXNodeType* node) const = 0;
virtual AXTreeID GetTreeID(AXNodeType* node) const = 0;
// Returns the length of text that this anchor node takes up in its parent. // Returns the length of text that this anchor node takes up in its parent.
// On some platforms, embedded objects are represented in their parent with a // On some platforms, embedded objects are represented in their parent with a
......
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