Skip to content

Commit

Permalink
Revert "Micro-optimizations in CollectMatchingRulesForListInternal."
Browse files Browse the repository at this point in the history
This reverts commit cf2358b.

Reason for revert: Causes memory regressions.

Original change's description:
> Micro-optimizations in CollectMatchingRulesForListInternal.
>
> Combine four optimizations that are too small to measure individually,
> but that together benchmark positive:
>
>  - Make CascadeLayerMap::kImplicitOuterLayerOrder a true constant,
>    so that the compiler doesn't have to load it from the binary
>    every time. (Fix resulting warning coming from implicit cast.)
>  - Don't count number of rejected rules separately; calculate it
>    at the end instead if needed. (This saves a register.)
>  - Don't refcount result.custom_highlight_name, especially as it's
>    almost never actually set.
>  - Store the Bloom filter directly in SelectorFilter, instead of
>    going through a pointer indirection every time. This also keeps
>    it from being allocated and deallocated every time we go hit the
>    uppermost element; we just clear it instead.
>
> Style microbenchmarks (Zen 3, LTO but no PGO):
>
> Initial style (µs)     Before     After    Perf      95% CI (BCa)
> =================== ========= ========= ======= =================
> ECommerce                8041      8015   +0.3%  [ -0.6%,  +1.0%]
> Encyclopedia            71498     70901   +0.8%  [ +0.2%,  +1.4%]
> Extension              166003    164651   +0.8%  [ +0.3%,  +1.3%]
> News                    31801     31391   +1.3%  [ +0.5%,  +1.8%]
> Search                   2164      2149   +0.7%  [ -0.1%,  +1.4%]
> Social1                 18797     18510   +1.5%  [ +0.8%,  +2.3%]
> Social2                   797       795   +0.3%  [ -0.4%,  +0.9%]
> Sports                  26062     26132   -0.3%  [ -0.9%,  +0.3%]
> Video                   31769     31868   -0.3%  [ -0.9%,  +0.4%]
> Geometric mean                            +0.6%  [ +0.3%,  +0.7%]
>
> Recalc style (µs)      Before     After    Perf      95% CI (BCa)
> =================== ========= ========= ======= =================
> ECommerce                9250      9240   +0.1%  [ -0.8%,  +0.7%]
> Encyclopedia            63747     63213   +0.8%  [ +0.1%,  +1.6%]
> Extension              164301    163611   +0.4%  [ -0.2%,  +0.9%]
> News                    23482     23118   +1.6%  [ +0.7%,  +2.2%]
> Search                    182       181   +0.5%  [ -0.9%,  +1.9%]
> Social1                 13178     12913   +2.0%  [ +1.4%,  +2.7%]
> Social2                   395       392   +0.7%  [ -0.2%,  +2.1%]
> Sports                  19213     19090   +0.6%  [ -0.1%,  +1.2%]
> Video                   19874     19708   +0.8%  [ -0.1%,  +1.5%]
> Geometric mean                            +0.9%  [ +0.5%,  +1.1%]
>
> Change-Id: I5aba1a57bdcfeba735154be814e1bd2532816e02
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3810556
> Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
> Commit-Queue: Steinar H Gunderson <sesse@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1031876}

Change-Id: I537af3d9563471e5fd35fa80ca7655ba83a57a47
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3812172
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Commit-Queue: Steinar H Gunderson <sesse@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1032477}
  • Loading branch information
Steinar H Gunderson authored and Chromium LUCI CQ committed Aug 8, 2022
1 parent 15bc781 commit b8fe5c9
Show file tree
Hide file tree
Showing 8 changed files with 32 additions and 25 deletions.
4 changes: 4 additions & 0 deletions third_party/blink/renderer/core/css/cascade_layer_map.cc
Expand Up @@ -7,6 +7,10 @@
#include "third_party/blink/renderer/core/css/rule_set.h"

namespace blink {

const unsigned CascadeLayerMap::kImplicitOuterLayerOrder =
std::numeric_limits<unsigned>::max();

namespace {

using CanonicalLayerMap =
Expand Down
3 changes: 1 addition & 2 deletions third_party/blink/renderer/core/css/cascade_layer_map.h
Expand Up @@ -17,8 +17,7 @@ namespace blink {
// layers in each sheet to the sorted layer order number.
class CORE_EXPORT CascadeLayerMap : public GarbageCollected<CascadeLayerMap> {
public:
static constexpr unsigned kImplicitOuterLayerOrder =
std::numeric_limits<unsigned>::max();
static const unsigned kImplicitOuterLayerOrder;

explicit CascadeLayerMap(const ActiveStyleSheetVector&);

Expand Down
11 changes: 7 additions & 4 deletions third_party/blink/renderer/core/css/element_rule_collector.cc
Expand Up @@ -351,13 +351,14 @@ void ElementRuleCollector::CollectMatchingRulesForListInternal(
rule_set->ContainerQueryIntervals());
Seeker<StyleScope> scope_seeker(rule_set->ScopeIntervals());

unsigned rejected = 0;
unsigned fast_rejected = 0;
unsigned matched = 0;
SelectorStatisticsCollector selector_statistics_collector;
if (perf_trace_enabled)
selector_statistics_collector.ReserveCapacity(rules->size());

for (const RuleData& rule_data : *rules) {
for (const auto& rule_data : *rules) {
if (perf_trace_enabled) {
selector_statistics_collector.EndCollectionForCurrentRule();
selector_statistics_collector.BeginCollectionForRule(&rule_data);
Expand All @@ -381,6 +382,7 @@ void ElementRuleCollector::CollectMatchingRulesForListInternal(
if (UNLIKELY(part_request && part_request->for_shadow_pseudo)) {
if (!selector.IsAllowedAfterPart()) {
DCHECK_EQ(selector.GetPseudoType(), CSSSelector::kPseudoPart);
rejected++;
continue;
}
DCHECK_EQ(selector.Relation(), CSSSelector::kUAShadow);
Expand All @@ -394,10 +396,12 @@ void ElementRuleCollector::CollectMatchingRulesForListInternal(
DCHECK(!context.is_inside_visited_link ||
inside_link_ != EInsideLink::kNotInsideLink);
if (!checker.Match(context, result)) {
rejected++;
continue;
}
if (pseudo_style_request_.pseudo_id != kPseudoIdNone &&
pseudo_style_request_.pseudo_id != result.dynamic_pseudo) {
rejected++;
continue;
}
const ContainerQuery* container_query =
Expand All @@ -417,6 +421,7 @@ void ElementRuleCollector::CollectMatchingRulesForListInternal(
result.dynamic_pseudo == kPseudoIdNone) {
if (!EvaluateAndAddContainerQueries(*container_query,
style_recalc_context_, result_)) {
rejected++;
if (AffectsAnimations(rule_data))
result_.SetConditionallyAffectsAnimations();
continue;
Expand All @@ -442,7 +447,6 @@ void ElementRuleCollector::CollectMatchingRulesForListInternal(
if (!style_engine.Stats())
return;

unsigned rejected = rules->size() - fast_rejected - matched;
INCREMENT_STYLE_STATS_COUNTER(style_engine, rules_rejected, rejected);
INCREMENT_STYLE_STATS_COUNTER(style_engine, rules_fast_rejected,
fast_rejected);
Expand Down Expand Up @@ -877,8 +881,7 @@ void ElementRuleCollector::DidMatchRule(

if (dynamic_pseudo == kPseudoIdHighlight) {
DCHECK(result.custom_highlight_name);
style_->SetHasCustomHighlightName(
AtomicString(result.custom_highlight_name));
style_->SetHasCustomHighlightName(result.custom_highlight_name);
}
} else if (dynamic_pseudo == kPseudoIdFirstLine && container_query) {
style_->SetFirstLineDependsOnSizeContainerQueries(true);
Expand Down
Expand Up @@ -1433,7 +1433,7 @@ TEST_F(StyleResolverTest, NoCascadeLayers) {
ASSERT_EQ(properties.size(), 3u);

const uint16_t kImplicitOuterLayerOrder =
ClampTo<uint16_t>(CascadeLayerMap::kImplicitOuterLayerOrder);
CascadeLayerMap::kImplicitOuterLayerOrder;

// div { display: block; }
EXPECT_TRUE(properties[0].properties->HasProperty(CSSPropertyID::kDisplay));
Expand Down Expand Up @@ -1480,7 +1480,7 @@ TEST_F(StyleResolverTest, CascadeLayersInDifferentSheets) {
ASSERT_EQ(properties.size(), 4u);

const uint16_t kImplicitOuterLayerOrder =
ClampTo<uint16_t>(CascadeLayerMap::kImplicitOuterLayerOrder);
CascadeLayerMap::kImplicitOuterLayerOrder;

// div { display: block; }
EXPECT_TRUE(properties[0].properties->HasProperty(CSSPropertyID::kDisplay));
Expand Down Expand Up @@ -1539,7 +1539,7 @@ TEST_F(StyleResolverTest, CascadeLayersInDifferentTreeScopes) {
ASSERT_EQ(properties.size(), 3u);

const uint16_t kImplicitOuterLayerOrder =
ClampTo<uint16_t>(CascadeLayerMap::kImplicitOuterLayerOrder);
CascadeLayerMap::kImplicitOuterLayerOrder;

// div { display: block }
EXPECT_TRUE(properties[0].properties->HasProperty(CSSPropertyID::kDisplay));
Expand Down Expand Up @@ -1593,7 +1593,7 @@ TEST_F(StyleResolverTest, CascadeLayersAfterModifyingAnotherSheet) {
ASSERT_EQ(properties.size(), 2u);

const uint16_t kImplicitOuterLayerOrder =
ClampTo<uint16_t>(CascadeLayerMap::kImplicitOuterLayerOrder);
CascadeLayerMap::kImplicitOuterLayerOrder;

// @layer { target { color: red } }"
EXPECT_TRUE(properties[0].properties->HasProperty(CSSPropertyID::kColor));
Expand Down
2 changes: 1 addition & 1 deletion third_party/blink/renderer/core/css/selector_checker.cc
Expand Up @@ -1661,7 +1661,7 @@ bool SelectorChecker::CheckPseudoElement(const SelectorCheckingContext& context,
// elements we have a single flag for tracking whether an element may
// match _any_ ::highlight() element (kPseudoIdHighlight).
if (!pseudo_argument_ || pseudo_argument_ == selector.Argument()) {
result.custom_highlight_name = selector.Argument().Impl();
result.custom_highlight_name = selector.Argument();
return true;
}
return false;
Expand Down
6 changes: 1 addition & 5 deletions third_party/blink/renderer/core/css/selector_checker.h
Expand Up @@ -191,11 +191,7 @@ class CORE_EXPORT SelectorChecker {

public:
PseudoId dynamic_pseudo{kPseudoIdNone};

// Comes from an AtomicString, but not stored as one to avoid
// the cost of checking the refcount on cleaning up from every
// Match() call. Owned by the CSS selector it came from.
StringImpl* custom_highlight_name{nullptr};
AtomicString custom_highlight_name;

// From the :has() argument selector checking, we need to get the element
// that matches the leftmost compound selector to mark all possible :has()
Expand Down
16 changes: 9 additions & 7 deletions third_party/blink/renderer/core/css/selector_filter.cc
Expand Up @@ -120,6 +120,7 @@ inline void CollectDescendantSelectorIdentifierHashes(
} // namespace

void SelectorFilter::PushParentStackFrame(Element& parent) {
DCHECK(ancestor_identifier_filter_);
DCHECK(parent_stack_.IsEmpty() ||
parent_stack_.back().element ==
FlatTreeTraversal::ParentElement(parent));
Expand All @@ -131,21 +132,22 @@ void SelectorFilter::PushParentStackFrame(Element& parent) {
CollectElementIdentifierHashes(parent, parent_frame.identifier_hashes);
wtf_size_t count = parent_frame.identifier_hashes.size();
for (wtf_size_t i = 0; i < count; ++i)
ancestor_identifier_filter_.Add(parent_frame.identifier_hashes[i]);
ancestor_identifier_filter_->Add(parent_frame.identifier_hashes[i]);
}

void SelectorFilter::PopParentStackFrame() {
DCHECK(!parent_stack_.IsEmpty());
DCHECK(ancestor_identifier_filter_);
const ParentStackFrame& parent_frame = parent_stack_.back();
wtf_size_t count = parent_frame.identifier_hashes.size();
for (wtf_size_t i = 0; i < count; ++i)
ancestor_identifier_filter_.Remove(parent_frame.identifier_hashes[i]);
ancestor_identifier_filter_->Remove(parent_frame.identifier_hashes[i]);
parent_stack_.pop_back();
if (parent_stack_.IsEmpty()) {
#if DCHECK_IS_ON()
DCHECK(ancestor_identifier_filter_.LikelyEmpty());
DCHECK(ancestor_identifier_filter_->LikelyEmpty());
#endif
ancestor_identifier_filter_.Clear();
ancestor_identifier_filter_.reset();
}
}

Expand All @@ -154,12 +156,12 @@ void SelectorFilter::PushParent(Element& parent) {
DCHECK(parent.InActiveDocument());
if (parent_stack_.IsEmpty()) {
DCHECK_EQ(parent, parent.GetDocument().documentElement());
#if DCHECK_IS_ON()
DCHECK(ancestor_identifier_filter_.IsClear());
#endif
DCHECK(!ancestor_identifier_filter_);
ancestor_identifier_filter_ = std::make_unique<IdentifierFilter>();
PushParentStackFrame(parent);
return;
}
DCHECK(ancestor_identifier_filter_);
// We may get invoked for some random elements in some wacky cases during
// style resolve. Pause maintaining the stack in this case.
if (parent_stack_.back().element != FlatTreeTraversal::ParentElement(parent))
Expand Down
7 changes: 5 additions & 2 deletions third_party/blink/renderer/core/css/selector_filter.h
Expand Up @@ -31,6 +31,8 @@
#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_CSS_SELECTOR_FILTER_H_
#define THIRD_PARTY_BLINK_RENDERER_CORE_CSS_SELECTOR_FILTER_H_

#include <memory>

#include "third_party/blink/renderer/core/core_export.h"
#include "third_party/blink/renderer/core/dom/element.h"
#include "third_party/blink/renderer/platform/wtf/bloom_filter.h"
Expand Down Expand Up @@ -118,15 +120,16 @@ class CORE_EXPORT SelectorFilter {
// With 100 unique strings in the filter, 2^12 slot table has false positive
// rate of ~0.2%.
using IdentifierFilter = CountingBloomFilter<12>;
IdentifierFilter ancestor_identifier_filter_;
std::unique_ptr<IdentifierFilter> ancestor_identifier_filter_;
};

template <unsigned maximumIdentifierCount>
inline bool SelectorFilter::FastRejectSelector(
const unsigned* identifier_hashes) const {
DCHECK(ancestor_identifier_filter_);
for (unsigned n = 0; n < maximumIdentifierCount && identifier_hashes[n];
++n) {
if (!ancestor_identifier_filter_.MayContain(identifier_hashes[n]))
if (!ancestor_identifier_filter_->MayContain(identifier_hashes[n]))
return true;
}
return false;
Expand Down

0 comments on commit b8fe5c9

Please sign in to comment.