Skip to content

Commit

Permalink
style: Use only Origin during the cascade, rather than CascadeLevel.
Browse files Browse the repository at this point in the history
The micro-benchmark `style-attr-1.html` regressed slightly with my patch, after
the CascadeLevel size increase.

This benchmark is meant to test for the "changing the style attribute doesn't
cause selector-matching" optimization (which, mind you, keeps working).

But in the process it creates 10k rules which form a perfect path in the rule
tree and that we put into a SmallVec during the cascade, and the benchmark
spends most of the time pushing to that SmallVec and iterating the declarations
(as there's only one property to apply).

So we could argue that the regression is minor and is not what the benchark is
supposed to be testing, but given I did the digging... :)

My patch made CascadeLevel bigger, which means that we create a somewhat bigger
vector in this case. Thankfully it also removed the dependency in the
CascadeLevel, so we can stop using that and use just Origin which is one byte to
revert the perf regression.

Differential Revision: https://phabricator.services.mozilla.com/D53181
  • Loading branch information
emilio committed Nov 30, 2019
1 parent 45c64a7 commit 425025c
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 23 deletions.
4 changes: 2 additions & 2 deletions components/style/animation.rs
Expand Up @@ -16,8 +16,8 @@ use crate::properties::animated_properties::AnimatedProperty;
use crate::properties::longhands::animation_direction::computed_value::single_value::T as AnimationDirection;
use crate::properties::longhands::animation_play_state::computed_value::single_value::T as AnimationPlayState;
use crate::properties::{self, CascadeMode, ComputedValues, LonghandId};
use crate::rule_tree::CascadeLevel;
use crate::stylesheets::keyframes_rule::{KeyframesAnimation, KeyframesStep, KeyframesStepValue};
use crate::stylesheets::Origin;
use crate::timer::Timer;
use crate::values::computed::box_::TransitionProperty;
use crate::values::computed::Time;
Expand Down Expand Up @@ -491,7 +491,7 @@ where
guard
.normal_declaration_iter()
.filter(|declaration| declaration.is_animatable())
.map(|decl| (decl, CascadeLevel::Animations))
.map(|decl| (decl, Origin::Author))
};

// This currently ignores visited styles, which seems acceptable,
Expand Down
36 changes: 19 additions & 17 deletions components/style/properties/cascade.rs
Expand Up @@ -15,7 +15,7 @@ use crate::properties::{LonghandId, LonghandIdSet, CSSWideKeyword};
use crate::properties::{PropertyDeclaration, PropertyDeclarationId, DeclarationImportanceIterator};
use crate::properties::CASCADE_PROPERTY;
use crate::rule_cache::{RuleCache, RuleCacheConditions};
use crate::rule_tree::{CascadeLevel, StrongRuleNode};
use crate::rule_tree::StrongRuleNode;
use crate::selector_parser::PseudoElement;
use crate::stylesheets::{Origin, PerOrigin};
use servo_arc::Arc;
Expand Down Expand Up @@ -134,11 +134,15 @@ where
let restriction = pseudo.and_then(|p| p.property_restriction());
let iter_declarations = || {
rule_node.self_and_ancestors().flat_map(|node| {
let cascade_level = node.cascade_level();
let origin = node.cascade_level().origin();
let node_importance = node.importance();
let guard = match origin {
Origin::Author => guards.author,
Origin::User | Origin::UserAgent => guards.ua_or_user,
};
let declarations = match node.style_source() {
Some(source) => source
.read(cascade_level.guard(guards))
.read(guard)
.declaration_importance_iter(),
None => DeclarationImportanceIterator::new(&[], &empty),
};
Expand All @@ -153,14 +157,14 @@ where
// longhands are only allowed if they have our
// restriction flag set.
if let PropertyDeclarationId::Longhand(id) = declaration.id() {
if !id.flags().contains(restriction) && cascade_level.origin() != Origin::UserAgent {
if !id.flags().contains(restriction) && origin != Origin::UserAgent {
return None;
}
}
}

if declaration_importance == node_importance {
Some((declaration, cascade_level))
Some((declaration, origin))
} else {
None
}
Expand Down Expand Up @@ -223,7 +227,7 @@ pub fn apply_declarations<'a, E, F, I>(
where
E: TElement,
F: Fn() -> I,
I: Iterator<Item = (&'a PropertyDeclaration, CascadeLevel)>,
I: Iterator<Item = (&'a PropertyDeclaration, Origin)>,
{
debug_assert!(layout_parent_style.is_none() || parent_style.is_some());
debug_assert_eq!(
Expand All @@ -242,17 +246,17 @@ where

let inherited_style = parent_style.unwrap_or(device.default_computed_values());

let mut declarations = SmallVec::<[(&_, CascadeLevel); 32]>::new();
let mut declarations = SmallVec::<[(&_, Origin); 32]>::new();
let custom_properties = {
let mut builder = CustomPropertiesBuilder::new(
inherited_style.custom_properties(),
device.environment(),
);

for (declaration, cascade_level) in iter_declarations() {
declarations.push((declaration, cascade_level));
for (declaration, origin) in iter_declarations() {
declarations.push((declaration, origin));
if let PropertyDeclaration::Custom(ref declaration) = *declaration {
builder.cascade(declaration, cascade_level.origin());
builder.cascade(declaration, origin);
}
}

Expand Down Expand Up @@ -332,16 +336,15 @@ where
fn should_ignore_declaration_when_ignoring_document_colors(
device: &Device,
longhand_id: LonghandId,
cascade_level: CascadeLevel,
origin: Origin,
pseudo: Option<&PseudoElement>,
declaration: &mut Cow<PropertyDeclaration>,
) -> bool {
if !longhand_id.ignored_when_document_colors_disabled() {
return false;
}

let is_ua_or_user_rule =
matches!(cascade_level.origin(), Origin::User | Origin::UserAgent);
let is_ua_or_user_rule = matches!(origin, Origin::User | Origin::UserAgent);
if is_ua_or_user_rule {
return false;
}
Expand Down Expand Up @@ -446,7 +449,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
declarations: I,
) where
Phase: CascadePhase,
I: Iterator<Item = (&'decls PropertyDeclaration, CascadeLevel)>,
I: Iterator<Item = (&'decls PropertyDeclaration, Origin)>,
{
let apply_reset = apply_reset == ApplyResetProperties::Yes;

Expand All @@ -459,9 +462,8 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {

let ignore_colors = !self.context.builder.device.use_document_colors();

for (declaration, cascade_level) in declarations {
for (declaration, origin) in declarations {
let declaration_id = declaration.id();
let origin = cascade_level.origin();
let longhand_id = match declaration_id {
PropertyDeclarationId::Longhand(id) => id,
PropertyDeclarationId::Custom(..) => continue,
Expand Down Expand Up @@ -509,7 +511,7 @@ impl<'a, 'b: 'a> Cascade<'a, 'b> {
let should_ignore = should_ignore_declaration_when_ignoring_document_colors(
self.context.builder.device,
longhand_id,
cascade_level,
origin,
self.context.builder.pseudo,
&mut declaration,
);
Expand Down
9 changes: 5 additions & 4 deletions components/style/stylist.rs
Expand Up @@ -1322,16 +1322,17 @@ impl Stylist {
let iter_declarations = || {
block
.declaration_importance_iter()
.map(|(declaration, importance)| {
debug_assert!(!importance.important());
(declaration, CascadeLevel::same_tree_author_normal())
})
.map(|(declaration, _)| (declaration, Origin::Author))
};

let metrics = get_metrics_provider_for_product();

// We don't bother inserting these declarations in the rule tree, since
// it'd be quite useless and slow.
//
// TODO(emilio): Now that we fixed bug 1493420, we should consider
// reversing this as it shouldn't be slow anymore, and should avoid
// generating two instantiations of apply_declarations.
properties::apply_declarations::<E, _, _>(
&self.device,
/* pseudo = */ None,
Expand Down

0 comments on commit 425025c

Please sign in to comment.