[tstreule]

Fixes #796. Part of #289.

Problem

Until now Tokenizer::finish_attribute scanned every attribute already on the tag for each new name. So one tag with n attributes costs O(n²).

Solution

Following what's proposed in #796, we adjust the tokenizer to mirror how Blink does it (kMinimumNumAttributesToDedupWithHash in atomic_html_token.h):

• <10 attributes: same linear scan, no behavior change.
• ≥10 attributes: looks the name up in a set of the tag's names (HashSet<LocalName>), one step regardless of how many attributes there are.

Changed.
The tokenizer in html5ever/src/tokenizer/mod.rs gets a new field current_tag_attr_names: RefCell<HashSet<LocalName>> and method is_duplicate_attribute, which returns whether the tag already has an attribute with that name.

Behavior is unchanged: the first occurrence wins, later ones are dropped with the Duplicate attribute parse error, and had_duplicate_attributes is set.

Tests

Tags with fewer than 10 attributes still take the linear scan, which the html5lib tokenizer tests already cover. Four new unit tests in tokenizer::test cover the set:

• duplicate_of_an_attribute_before_the_threshold: a duplicate of a0 on tags with 9, 10 and 11 attributes. From 10 on, a0 is found among the names put into the set when it is first filled.
• duplicate_of_an_attribute_after_the_threshold: id=first id=second after 12 attributes. id is found among the names added to the set one by one.
• no_duplicates_on_a_next_tag_with_the_same_attributes: two tags with the same 12 attributes report no duplicates, so the set is cleared between tags.
• duplicate_on_a_next_tag_with_other_attributes: a duplicate on a second tag with other names is still found, so the set is filled again for each tag.

They check the kept attributes, the Duplicate attribute errors and had_duplicate_attributes. The threshold is a module-level constant, MIN_ATTRS_TO_DEDUP_WITH_HASH, so the tests follow it if it changes.

Timing

The program.

use html5ever::tendril::StrTendril;
use html5ever::tokenizer::{BufferQueue, Token, TokenSink, TokenSinkResult, Tokenizer, TokenizerOpts};
use std::{cell::Cell, time::Instant};

struct Count(Cell<usize>);
impl TokenSink for Count {
    type Handle = ();
    fn process_token(&self, t: Token, _line: u64) -> TokenSinkResult<()> {
        if let Token::TagToken(tag) = t { self.0.set(self.0.get() + tag.attrs.len()); }
        TokenSinkResult::Continue
    }
}

fn main() {
    for n in [10_000usize, 20_000, 40_000, 80_000] {
        let mut s = String::from("<div");
        for i in 0..n { s.push_str(&format!(" a{i}=x")); }
        s.push_str("></div>");
        let tok = Tokenizer::new(Count(Cell::new(0)), TokenizerOpts::default());
        let q = BufferQueue::default();
        q.push_back(StrTendril::from(s.as_str()));
        let t0 = Instant::now();
        let _ = tok.feed(&q);
        tok.end();
        println!("{n} attrs: {:.3?}", t0.elapsed());
    }
}

Results.
Release build, Apple M1 Pro, 5 runs per side. Median, with the range in brackets:

attributes	main (7760920)	branch (6566229)
10'000	20.8 ms (20.5–25.7)	2.13 ms (2.05–2.47)
20'000	83.0 ms (81.1–83.9)	4.41 ms (4.24–4.86)
40'000	328 ms (322–330)	9.20 ms (9.02–9.52)
80'000	1.29 s (1.28–1.31)	18.5 ms (17.8–18.7)

While main grows about 4x per doubling on the attribute count (quadratic), this branch grows about 2x per doubling (linear).

[mrobinson]

@tstreule Can you verify that your change aligns with Servo AI contribution policy: https://book.servo.org/policy/ai-usage.html? In short, LLMs are fine for preparation / investigation / review, but no generated code, PR descriptions or comments are allowed.

[tstreule]

No AI was used for most of the work, like code and testing. However, I used AI to rename variables and clean up the PR description. If you like, I can gladly rewrite the PR description. Sorry for any inconvenience!

[mrobinson]

No AI was used for most of the work, like code and testing. However, I used AI to rename variables and clean up the PR description. If you like, I can gladly rewrite the PR description. Sorry for any inconvenience!

Thanks! Yes, if you don't mind please rewrite the description in your own words.

[tstreule]

No AI was used for most of the work, like code and testing. However, I used AI to rename variables and clean up the PR description. If you like, I can gladly rewrite the PR description. Sorry for any inconvenience!

Thanks! Yes, if you don't mind please rewrite the description in your own words.

@mrobinson Sorry that it took a bit longer. Should be ready for your review now.

[michiel]

Found this same issue, adding timing information for reference:

On html5ever 0.39.0, a single ~1 MiB end tag carrying ~150,000 distinct attribute names did not finish within 25 s. End tags go through the same finish_attribute path, so a sink-side limit can't prevent the cost.

With a per-tag HashSet<LocalName> for the duplicate check (same approach as this PR), the same inputs fell to well under a second. One caveat: with distinct names, interning then becomes the bottleneck when names are crafted to collide (servo/string-cache#314 / #315). With both fixes, 16 tags × 4,095 distinct names took ~0.18 s.

[notriddle]

It's not a good idea to name the function is_duplicate_attribute when it's mutating a data structure. The name makes it sound like it's idempotent, but it isn't.

Instead, I'd structure this method around appending to the list:

Suggested change

	fn is_duplicate_attribute(&self, name: &LocalName) -> bool {
	let attrs = self.current_tag_attrs.borrow();
	if attrs.len() < MIN_ATTRS_TO_DEDUP_WITH_HASH {
	return attrs.iter().any(|a| a.name.local == *name);
	}
	let mut names = self.current_tag_attr_names.borrow_mut();
	if names.is_empty() {
	names.extend(attrs.iter().map(|a| a.name.local.clone()));
	}
	!names.insert(name.clone())
	}
	fn add_to_current_tag_attrs(&self, name: &LocalName) -> bool {
	let mut attrs = self.current_tag_attrs.borrow_mut();
	let should_add =
	if attrs.len() < MIN_ATTRS_TO_DEDUP_WITH_HASH {
	!attrs.iter().any(|a| a.name.local == *name)
	} else {
	let mut names = self.current_tag_attr_names.borrow_mut();
	if names.is_empty() {
	names.extend(attrs.iter().map(|a| a.name.local.clone()));
	}
	names.insert(name.clone())
	};
	if should_add {
	attrs.push(Attribute {
	// The tree builder will adjust the namespace if necessary.
	// This only happens in foreign elements.
	name: QualName::new(None, ns!(), name),
	value: mem::take(&mut self.current_attr_value.borrow_mut()),
	})
	}
	should_add
	}

Then the spot where it's actually used becomes:

        if !self.add_to_current_tag_attrs(&name) {
            self.emit_error(Borrowed("Duplicate attribute"));
            self.current_tag_had_duplicate_attributes.set(true);
            self.current_attr_value.borrow_mut().clear();
        }