Add a one-token cache #171

SimonSapin · 2017-07-18T07:54:07Z

This makes the Servo_DeclarationBlock_SetPropertyById micro-benchmark being added in https://bugzilla.mozilla.org/show_bug.cgi?id=1344131 faster by ~10%.

This change is

This make the Clone impl never allocate.

This will avoid duplicating tokenization work, hopefully in most uses of `Parser::try`.

emilio · 2017-07-18T07:56:07Z

src/compact_cow_str.rs

 }

 impl<'a> From<&'a str> for CompactCowStr<'a> {
    #[inline]
    fn from(s: &'a str) -> Self {
        let len = s.len();
-        assert!(len <= MAX_LEN);
+        assert!(len < usize::MAX);


I think this would be (slightly) clearer as != usize::MAX.

emilio · 2017-07-18T07:57:52Z

src/compact_cow_str.rs

-    fn as_raw_str(&self) -> *const str {
-        unsafe {
-            str::from_utf8_unchecked(slice::from_raw_parts(self.ptr, self.len()))
+    fn unpack(&self) -> Result<&'a str, *const String> {


Could this please have some docs? I found the name not super-descriptive.

emilio · 2017-07-18T08:04:44Z

src/parser.rs

+                    // ```
+                    // parser.try(|parser| {
+                    //     match parser.next() { … }
+                    // }).or_else(|| {


Won't this change behaviour if I do:

parser.try(|p| match parser.next_including_whitespace_and_comments() { .. }) .or_else(|| match parser.next_including_whitespace_and_comments() { .. })

AFAIU, the position won't be the same after the whitespace token and thus the cache won't be used.

SimonSapin · 2017-07-18T08:17:39Z

SetPropertyById faster by ~10%.

It also makes full stylesheet parsing slower, so let’s not land this just yet :(

It’s pretty much useless now that the refcount is almost never one.

SimonSapin · 2017-07-21T16:31:26Z

Alright, in the current state of this PR (with a bunch of changes to the Servo repository for which I’ll open a PR next), gecko benchmarks changes are:

Full stylesheet parsing: unchanged (or within noise level)
Parsing (eCSSProperty_width, "10px"): ~15% faster
Parsing (eCSSProperty_width, " 10px") (note extra space): ~5% slower

The regression is unfortunate. An additional change (not caching whitespace tokens that are skipped by Parser::next) fixes it (makes the " 10px" bench ~15% faster than master) but also makes full stylesheet parsing ~20% slower, so I removed that change from the PR.

I’ll keep trying other things, but I’d still like to land this so that the Servo changes do not bitrot too much. r? @emilio

Update cssparser to 0.18 Do not merge yet, depends on servo/rust-cssparser#171.

Update cssparser to 0.18 Do not merge yet, depends on servo/rust-cssparser#171.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17820)

emilio

(Just a few comments, will review properly tomorrow)

emilio · 2017-07-21T20:34:54Z

src/tokenizer.rs

@@ -245,6 +245,15 @@ impl<'a> Tokenizer<'a> {
    }

    #[inline]
+    pub fn see_function(&mut self, name: &str) {
+        if self.var_functions == SeenStatus::LookingForThem {


nit: I'd probably use early-return here.

emilio · 2017-07-21T20:37:16Z

src/color.rs

@@ -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();


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?

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.

emilio · 2017-07-21T20:39:16Z

src/color.rs

@@ -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() {


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?

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.

emilio

Looks good, I wonder if we could make a Rc<str> instead of an Rc<String>, which AIUI double-allocates...

SimonSapin · 2017-07-22T18:32:45Z

rust-lang/rust#42565 adds From<&str> for Rc<str> but has not landed yet. Also, Rc<str> is a fat pointer, which would make this bit packing more tricky. (Rc::<T>::into_raw and Rc::<T>::from_raw are only available for T: Sized.)

SimonSapin · 2017-07-23T13:28:53Z

Ideally we’d want a growable RcString that contains refcount, length, capacity, and data all in one allocation. I tried it, but the amount of rarely-exercised unsafe code this required didn’t seem worth it.

emilio · 2017-07-23T14:48:19Z

Yeah, fair enough... Anyway, feel free to land with r=me when you feel comfortable with it.

SimonSapin · 2017-07-23T16:40:03Z

Cool, thanks. Maybe servo/servo#17820 should be ready too before we land this one. It has a green servo try run but still need review. Do you want to take it as well? It’s fairly mechanical.

SimonSapin · 2017-07-24T09:29:03Z

@bors-servo r=emilio

bors-servo · 2017-07-24T09:29:04Z

📌 Commit 850eac1 has been approved by emilio

bors-servo · 2017-07-24T09:29:07Z

⌛ Testing commit 850eac1 with merge efcfb37...

Add a one-token cache This makes the `Servo_DeclarationBlock_SetPropertyById` micro-benchmark being added in https://bugzilla.mozilla.org/show_bug.cgi?id=1344131 faster by ~10%.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-cssparser/171)

bors-servo · 2017-07-24T09:32:55Z

☀️ Test successful - status-travis
Approved by: emilio
Pushing efcfb37 to master...

servo/rust-cssparser#171

Update cssparser to 0.18 Do not merge yet, depends on servo/rust-cssparser#171.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17820)

…e); r=emilio Do not merge yet, depends on servo/rust-cssparser#171. Source-Repo: https://github.com/servo/servo Source-Revision: 4f0821192c112943bb53b4fb04303c1afdde06e6 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : e4825d4514cfe2286e2f874b4bd643befe603521

…e); r=emilio Do not merge yet, depends on servo/rust-cssparser#171. Source-Repo: https://github.com/servo/servo Source-Revision: 4f0821192c112943bb53b4fb04303c1afdde06e6

…e); r=emilio Do not merge yet, depends on servo/rust-cssparser#171. Source-Repo: https://github.com/servo/servo Source-Revision: 4f0821192c112943bb53b4fb04303c1afdde06e6 UltraBlame original commit: e5aad4922fba8855a4bfc979adf4e56c44d6db8c

SimonSapin added 8 commits July 18, 2017 09:52

Replace Box<str> with Rc<String> in CompactCowStr.

3b1906b

This make the Clone impl never allocate.

Rename CompactCowStr to CowRcStr

f4cfca9

Remove CowRcStr APIs that might allocate and used to never do so.

32733fb

Add From<Cow<str>> for CowRcStr

8b7191b

Rename compact_cow_str.rs to cow_rc_str.rs

c661e7c

Remove unnecessary parenthesis

4aeca59

Rename private struct fields in ParserInput and Parser

49e8ab4

Add a one-token cache.

b6072ca

This will avoid duplicating tokenization work, hopefully in most uses of `Parser::try`.

emilio reviewed Jul 18, 2017

View reviewed changes

SimonSapin added 5 commits July 20, 2017 22:19

Token cache heuristic: monotonic position rather than token type.

4705393

Make Parser::next return &Token instead of Token

a79e536

Return &CowRcStr instead of CowRcStr in some Parser::expect_* methods

c305bcf

Add expect_ident_cloned and expect_string_cloned

b806c39

Remove CowRcStr::into_owned

336a12e

It’s pretty much useless now that the refcount is almost never one.

SimonSapin force-pushed the token-cache branch from 999c0fc to 336a12e Compare July 21, 2017 16:25

SimonSapin mentioned this pull request Jul 21, 2017

Update cssparser to 0.18 servo/servo#17820

Merged

bors-servo pushed a commit to servo/servo that referenced this pull request Jul 21, 2017

Auto merge of #17820 - servo:token-cache, r=<try>

c3738e8

Update cssparser to 0.18 Do not merge yet, depends on servo/rust-cssparser#171.

Fix Parser::seen_* in the presence of caching

a944f54

emilio reviewed Jul 21, 2017

View reviewed changes

Remove unnecesseray clone

850eac1

SimonSapin mentioned this pull request Jul 22, 2017

'cargo update -p foo' fails with [patch]’ed depenedency rust-lang/cargo#4315

Closed

emilio approved these changes Jul 22, 2017

View reviewed changes

bors-servo merged commit 850eac1 into master Jul 24, 2017

SimonSapin deleted the token-cache branch July 24, 2017 09:33

SimonSapin added a commit to servo/servo that referenced this pull request Jul 24, 2017

Update cssparser to 0.18

3f7decd

servo/rust-cssparser#171

SimonSapin added a commit to servo/servo that referenced this pull request Jul 24, 2017

Update cssparser to 0.18

eb98ae6

servo/rust-cssparser#171

murarth mentioned this pull request Aug 24, 2017

Implement Arc/Rc raw pointer conversions for ?Sized rust-lang/rust#44073

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a one-token cache #171

Add a one-token cache #171

SimonSapin commented Jul 18, 2017 •

edited by larsbergstrom

Loading

emilio Jul 18, 2017

emilio Jul 18, 2017

emilio Jul 18, 2017

nox Jul 18, 2017

emilio Jul 18, 2017

SimonSapin commented Jul 18, 2017 •

edited

Loading

SimonSapin commented Jul 21, 2017 •

edited

Loading

emilio left a comment

emilio Jul 21, 2017

emilio Jul 21, 2017

emilio Jul 21, 2017

SimonSapin Jul 21, 2017

emilio Jul 21, 2017

SimonSapin Jul 21, 2017

emilio left a comment

SimonSapin commented Jul 22, 2017

SimonSapin commented Jul 23, 2017

emilio commented Jul 23, 2017

SimonSapin commented Jul 23, 2017

SimonSapin commented Jul 24, 2017

bors-servo commented Jul 24, 2017

bors-servo commented Jul 24, 2017

bors-servo commented Jul 24, 2017

Add a one-token cache #171

Add a one-token cache #171

Conversation

SimonSapin commented Jul 18, 2017 • edited by larsbergstrom Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonSapin commented Jul 18, 2017 • edited Loading

SimonSapin commented Jul 21, 2017 • edited Loading

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio left a comment

Choose a reason for hiding this comment

SimonSapin commented Jul 22, 2017

SimonSapin commented Jul 23, 2017

emilio commented Jul 23, 2017

SimonSapin commented Jul 23, 2017

SimonSapin commented Jul 24, 2017

bors-servo commented Jul 24, 2017

bors-servo commented Jul 24, 2017

bors-servo commented Jul 24, 2017

SimonSapin commented Jul 18, 2017 •

edited by larsbergstrom

Loading

SimonSapin commented Jul 18, 2017 •

edited

Loading

SimonSapin commented Jul 21, 2017 •

edited

Loading