Skip to content

Commit

Permalink
Avoid pointer-chasing to check whether there are any declarations.
Browse files Browse the repository at this point in the history
  • Loading branch information
@bholley
bholley committed Apr 22, 2017
1 parent 97c14f0 commit 1d6763c
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 43 deletions.
99 changes: 63 additions & 36 deletions components/style/stylist.rs
View file
Expand Up @@ -324,12 +324,11 @@ impl Stylist {
self.element_map.borrow_for_origin(&stylesheet.origin)
};

map.insert(Rule {
selector: selector.inner.clone(),
style_rule: locked.clone(),
specificity: selector.specificity,
source_order: self.rules_source_order,
});
map.insert(Rule::new(guard,
selector.inner.clone(),
locked.clone(),
self.rules_source_order,
selector.specificity));
}
self.rules_source_order += 1;

Expand Down Expand Up @@ -661,7 +660,6 @@ impl Stylist {
// Step 1: Normal user-agent rules.
map.user_agent.get_all_matching_rules(element,
parent_bf,
guards.ua_or_user,
applicable_declarations,
&mut relations,
flags_setter,
Expand All @@ -688,15 +686,13 @@ impl Stylist {
// Step 3: User and author normal rules.
map.user.get_all_matching_rules(element,
parent_bf,
guards.ua_or_user,
applicable_declarations,
&mut relations,
flags_setter,
CascadeLevel::UserNormal);
debug!("user normal: {:?}", relations);
map.author.get_all_matching_rules(element,
parent_bf,
guards.author,
applicable_declarations,
&mut relations,
flags_setter,
Expand Down Expand Up @@ -731,7 +727,6 @@ impl Stylist {
// Step 6: Author-supplied `!important` rules.
map.author.get_all_matching_rules(element,
parent_bf,
guards.author,
applicable_declarations,
&mut relations,
flags_setter,
Expand All @@ -755,7 +750,6 @@ impl Stylist {
// Step 8: User `!important` rules.
map.user.get_all_matching_rules(element,
parent_bf,
guards.ua_or_user,
applicable_declarations,
&mut relations,
flags_setter,
Expand All @@ -769,7 +763,6 @@ impl Stylist {
// Step 9: UA `!important` rules.
map.user_agent.get_all_matching_rules(element,
parent_bf,
guards.ua_or_user,
applicable_declarations,
&mut relations,
flags_setter,
Expand Down Expand Up @@ -990,7 +983,6 @@ impl SelectorMap {
pub fn get_all_matching_rules<E, V, F>(&self,
element: &E,
parent_bf: Option<&BloomFilter>,
guard: &SharedRwLockReadGuard,
matching_rules_list: &mut V,
relations: &mut StyleRelations,
flags_setter: &mut F,
Expand All @@ -1010,7 +1002,6 @@ impl SelectorMap {
parent_bf,
&self.id_hash,
&id,
guard,
matching_rules_list,
relations,
flags_setter,
Expand All @@ -1022,7 +1013,6 @@ impl SelectorMap {
parent_bf,
&self.class_hash,
class,
guard,
matching_rules_list,
relations,
flags_setter,
Expand All @@ -1038,7 +1028,6 @@ impl SelectorMap {
parent_bf,
local_name_hash,
element.get_local_name(),
guard,
matching_rules_list,
relations,
flags_setter,
Expand All @@ -1047,7 +1036,6 @@ impl SelectorMap {
SelectorMap::get_matching_rules(element,
parent_bf,
&self.other_rules,
guard,
matching_rules_list,
relations,
flags_setter,
Expand Down Expand Up @@ -1107,7 +1095,6 @@ impl SelectorMap {
parent_bf: Option<&BloomFilter>,
hash: &FnvHashMap<Str, Vec<Rule>>,
key: &BorrowedStr,
guard: &SharedRwLockReadGuard,
matching_rules: &mut Vector,
relations: &mut StyleRelations,
flags_setter: &mut F,
Expand All @@ -1122,7 +1109,6 @@ impl SelectorMap {
SelectorMap::get_matching_rules(element,
parent_bf,
rules,
guard,
matching_rules,
relations,
flags_setter,
Expand All @@ -1134,7 +1120,6 @@ impl SelectorMap {
fn get_matching_rules<E, V, F>(element: &E,
parent_bf: Option<&BloomFilter>,
rules: &[Rule],
guard: &SharedRwLockReadGuard,
matching_rules: &mut V,
relations: &mut StyleRelations,
flags_setter: &mut F,
Expand All @@ -1144,12 +1129,10 @@ impl SelectorMap {
F: FnMut(&E, ElementSelectorFlags),
{
for rule in rules.iter() {
let style_rule = rule.style_rule.read_with(guard);
let block = style_rule.block.read_with(guard);
let any_declaration_for_importance = if cascade_level.is_important() {
block.any_important()
rule.any_important_declarations()
} else {
block.any_normal()
rule.any_normal_declarations()
};
if any_declaration_for_importance &&
matches_selector(&rule.selector, element, parent_bf,
Expand Down Expand Up @@ -1230,33 +1213,77 @@ impl SelectorMap {
#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
#[derive(Clone, Debug)]
pub struct Rule {
/// The selector this struct represents.
/// This is an Arc because Rule will essentially be cloned for every element
/// that it matches. Selector contains an owned vector (through
/// ComplexSelector) and we want to avoid the allocation.
///
/// FIXME(emilio): We should be able to get rid of it and just use the style
/// rule? This predates the time where the rule was in `selectors`, and the
/// style rule was a generic parameter to it. It's not trivial though, due
/// to the specificity.
/// The selector this struct represents. We store this and the
/// any_{important,normal} booleans inline in the Rule to avoid
/// pointer-chasing when gathering applicable declarations, which
/// can ruin performance when there are a lot of rules.
#[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")]
pub selector: SelectorInner<SelectorImpl>,
/// The actual style rule.
#[cfg_attr(feature = "servo", ignore_heap_size_of = "Arc")]
pub style_rule: Arc<Locked<StyleRule>>,
/// The source order this style rule appears in.
pub source_order: usize,
/// The specificity of the rule this selector represents.
pub specificity: u32,
/// Bottom 30 bits: The specificity of the rule this selector represents.
/// 31st bit: Whether the rule's declaration block has any important declarations.
/// 32nd bit: Whether the rule's declaration block has any normal declarations.
specificity_and_bits: u32,
}

/// Masks for specificity_and_bits.
const SPECIFICITY_MASK: u32 = 0x3fffffff;
const ANY_IMPORTANT_DECLARATIONS_BIT: u32 = 1 << 30;
const ANY_NORMAL_DECLARATIONS_BIT: u32 = 1 << 31;

impl Rule {
/// Returns the specificity of the rule.
pub fn specificity(&self) -> u32 {
self.specificity_and_bits & SPECIFICITY_MASK
}

fn any_important_declarations(&self) -> bool {
(self.specificity_and_bits & ANY_IMPORTANT_DECLARATIONS_BIT) != 0
}

fn any_normal_declarations(&self) -> bool {
(self.specificity_and_bits & ANY_NORMAL_DECLARATIONS_BIT) != 0
}

fn to_applicable_declaration_block(&self, level: CascadeLevel) -> ApplicableDeclarationBlock {
ApplicableDeclarationBlock {
source: StyleSource::Style(self.style_rule.clone()),
level: level,
source_order: self.source_order,
specificity: self.specificity,
specificity: self.specificity(),
}
}

/// Creates a new Rule.
pub fn new(guard: &SharedRwLockReadGuard,
selector: SelectorInner<SelectorImpl>,
style_rule: Arc<Locked<StyleRule>>,
source_order: usize,
specificity: u32)
-> Self
{
let (any_important, any_normal) = {
let block = style_rule.read_with(guard).block.read_with(guard);
(block.any_important(), block.any_normal())
};
debug_assert!(specificity & (ANY_IMPORTANT_DECLARATIONS_BIT | ANY_NORMAL_DECLARATIONS_BIT) == 0);
let mut specificity_and_bits = specificity;
if any_important {
specificity_and_bits |= ANY_IMPORTANT_DECLARATIONS_BIT;
}
if any_normal {
specificity_and_bits |= ANY_NORMAL_DECLARATIONS_BIT;
}

Rule {
selector: selector,
style_rule: style_rule,
source_order: source_order,
specificity_and_bits: specificity_and_bits,
}
}
}
Expand Down
13 changes: 6 additions & 7 deletions tests/unit/style/stylist.rs
View file
Expand Up @@ -34,12 +34,11 @@ fn get_mock_rules(css_selectors: &[&str]) -> (Vec<Vec<Rule>>, SharedRwLock) {
let guard = shared_lock.read();
let rule = locked.read_with(&guard);
rule.selectors.0.iter().map(|s| {
Rule {
selector: s.inner.clone(),
style_rule: locked.clone(),
specificity: s.specificity,
source_order: i,
}
Rule::new(&guard,
s.inner.clone(),
locked.clone(),
i,
s.specificity)
}).collect()
}).collect(), shared_lock)
}
Expand All @@ -62,7 +61,7 @@ fn test_rule_ordering_same_specificity() {
let (rules_list, _) = get_mock_rules(&["a.intro", "img.sidebar"]);
let a = &rules_list[0][0];
let b = &rules_list[1][0];
assert!((a.specificity, a.source_order) < ((b.specificity, b.source_order)),
assert!((a.specificity(), a.source_order) < ((b.specificity(), b.source_order)),
"The rule that comes later should win.");
}

Expand Down

0 comments on commit 1d6763c

Please sign in to comment.