Skip to content

Add a one-token cache #171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Jul 24, 2017
Merged

Add a one-token cache #171

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
3b1906b
Replace Box<str> with Rc<String> in CompactCowStr.
SimonSapin Jul 4, 2017
f4cfca9
Rename CompactCowStr to CowRcStr
SimonSapin Jul 4, 2017
32733fb
Remove CowRcStr APIs that might allocate and used to never do so.
SimonSapin Jul 4, 2017
8b7191b
Add `From<Cow<str>> for CowRcStr`
SimonSapin Jul 18, 2017
c661e7c
Rename compact_cow_str.rs to cow_rc_str.rs
SimonSapin Jul 18, 2017
4aeca59
Remove unnecessary parenthesis
SimonSapin Jul 17, 2017
49e8ab4
Rename private struct fields in ParserInput and Parser
SimonSapin Jul 17, 2017
b6072ca
Add a one-token cache.
SimonSapin Jul 17, 2017
4705393
Token cache heuristic: monotonic position rather than token type.
SimonSapin Jul 18, 2017
a79e536
Make Parser::next return &Token instead of Token
SimonSapin Jul 18, 2017
c305bcf
Return &CowRcStr instead of CowRcStr in some Parser::expect_* methods
SimonSapin Jul 18, 2017
b806c39
Add expect_ident_cloned and expect_string_cloned
SimonSapin Jul 20, 2017
336a12e
Remove CowRcStr::into_owned
SimonSapin Jul 20, 2017
a944f54
Fix Parser::seen_* in the presence of caching
SimonSapin Jul 21, 2017
850eac1
Remove unnecesseray clone
SimonSapin Jul 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[package]

name = "cssparser"
version = "0.17.0"
version = "0.18.0"
authors = [ "Simon Sapin <[email protected]>" ]

description = "Rust implementation of CSS Syntax Level 3"
Expand Down
42 changes: 18 additions & 24 deletions src/color.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ impl Color {
///
/// FIXME(#2) Deprecated CSS2 System Colors are not supported yet.
pub fn parse<'i, 't>(input: &mut Parser<'i, 't>) -> Result<Color, BasicParseError<'i>> {
let token = input.next()?;
let token = input.next()?.clone();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this clone needed? couldn't you just use match *token? I guess maybe you can't parse the nested block below, is that it?

And, you need the clone below for the error?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cloning is needed so that input is not still borrowed by the time we call input.parse_nested_block in the Token::Function case below. This is language limitation, non-lexical lifetimes will hopefully make this clone unnecessary. I’ll add a comment.

You’re right though that this clone makes the other one unnecessary, I’ll remove that.

match token {
Token::Hash(ref value) | Token::IDHash(ref value) => {
Color::parse_hash(value.as_bytes())
Expand All @@ -154,7 +154,7 @@ impl Color {
}).map_err(ParseError::<()>::basic);
}
_ => Err(())
}.map_err(|()| BasicParseError::UnexpectedToken(token))
}.map_err(|()| BasicParseError::UnexpectedToken(token.clone()))
}

/// Parse a color hash, without the leading '#' character.
Expand Down Expand Up @@ -422,21 +422,17 @@ fn parse_color_function<'i, 't>(name: &str, arguments: &mut Parser<'i, 't>) -> R
if uses_commas {
arguments.expect_comma()?;
} else {
match arguments.next()? {
Token::Delim('/') => {},
t => return Err(BasicParseError::UnexpectedToken(t)),
};
arguments.expect_delim('/')?;
};
let token = arguments.next()?;
match token {
match *arguments.next()? {
Token::Number { value: v, .. } => {
clamp_unit_f32(v)
}
Token::Percentage { unit_value: v, .. } => {
clamp_unit_f32(v)
}
t => {
return Err(BasicParseError::UnexpectedToken(t))
ref t => {
return Err(BasicParseError::UnexpectedToken(t.clone()))
}
}
} else {
Expand All @@ -457,10 +453,10 @@ fn parse_rgb_components_rgb<'i, 't>(arguments: &mut Parser<'i, 't>) -> Result<(u

// Either integers or percentages, but all the same type.
// https://drafts.csswg.org/css-color/#rgb-functions
match arguments.next()? {
match arguments.next()?.clone() {
Token::Number { value: v, .. } => {
red = clamp_floor_256_f32(v);
green = clamp_floor_256_f32(match arguments.next()? {
green = clamp_floor_256_f32(match arguments.next()?.clone() {
Token::Number { value: v, .. } => v,
Token::Comma => {
uses_commas = true;
Expand All @@ -475,7 +471,7 @@ fn parse_rgb_components_rgb<'i, 't>(arguments: &mut Parser<'i, 't>) -> Result<(u
}
Token::Percentage { unit_value, .. } => {
red = clamp_unit_f32(unit_value);
green = clamp_unit_f32(match arguments.next()? {
green = clamp_unit_f32(match arguments.next()?.clone() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... This is kind of unfortunate, because it means that before we created a token, and now we need to create two copies of it... Does it happen too often? I wonder if that's why parsing a stylesheet becomes slower?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parsing initially became slower when Parser::next would clone every single token in order to put in the cache. Returning &Token removed these clones. In a minority of cases like here, the callers had to do these clones themselves instead. Note though that Token::clone never allocates. (Refcounting is used when a token contains an allocation.)

I don’t know the reason for the more recent regression. I’ll investigate next week.

Token::Percentage { unit_value, .. } => unit_value,
Token::Comma => {
uses_commas = true;
Expand All @@ -498,28 +494,26 @@ fn parse_rgb_components_hsl<'i, 't>(arguments: &mut Parser<'i, 't>) -> Result<(u
let mut uses_commas = false;
// Hue given as an angle
// https://drafts.csswg.org/css-values/#angles
let token = arguments.next()?;
let hue_degrees = match token {
Token::Number { value: v, .. } => Ok(v),
let hue_degrees = match *arguments.next()? {
Token::Number { value: v, .. } => v,
Token::Dimension { value: v, ref unit, .. } => {
match_ignore_ascii_case! { &*unit,
"deg" => Ok(v),
"grad" => Ok(v * 360. / 400.),
"rad" => Ok(v * 360. / (2. * PI)),
"turn" => Ok(v * 360.),
_ => Err(()),
"deg" => v,
"grad" => v * 360. / 400.,
"rad" => v * 360. / (2. * PI),
"turn" => v * 360.,
_ => return Err(BasicParseError::UnexpectedToken(Token::Ident(unit.clone()))),
}
}
t => return Err(BasicParseError::UnexpectedToken(t))
ref t => return Err(BasicParseError::UnexpectedToken(t.clone()))
};
let hue_degrees = hue_degrees.map_err(|()| BasicParseError::UnexpectedToken(token))?;
// Subtract an integer before rounding, to avoid some rounding errors:
let hue_normalized_degrees = hue_degrees - 360. * (hue_degrees / 360.).floor();
let hue = hue_normalized_degrees / 360.;

// Saturation and lightness are clamped to 0% ... 100%
// https://drafts.csswg.org/css-color/#the-hsl-notation
let saturation = match arguments.next()? {
let saturation = match arguments.next()?.clone() {
Token::Percentage { unit_value, .. } => unit_value,
Token::Comma => {
uses_commas = true;
Expand Down
246 changes: 0 additions & 246 deletions src/compact_cow_str.rs
View file
Open in desktop

This file was deleted.

Loading