Reduce the size of Token #159

SimonSapin · 2017-06-15T19:58:29Z

Hopefully improving parsing performance: https://bugzilla.mozilla.org/show_bug.cgi?id=1347408

This change is

Upgrade cssparser to 0.15 Depends on servo/rust-cssparser#159

Upgrade cssparser to 0.15 Depends on servo/rust-cssparser#159  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17345)

emilio

Looks good! Some comments.

emilio · 2017-06-15T20:59:07Z

src/compact_cow_str.rs

+    // * If the tag is 0, the memory is borrowed.
+    //   `ptr` and `len` are the components of a `&'a str`.
+
+    ptr: *const u8,


nit: Add a TODO for NonZero?

Sure, done. Although I think we never use this in Option or other places where NonZero would make a difference.

emilio · 2017-06-15T20:59:58Z

src/compact_cow_str.rs

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


Quick question, why add the tag to the length instead of the pointer?

Both seem fine, but with the second you get the benefit of not having the MAX_LENGTH restriction (though with the first you need to remove the tag).

At least in the current implementation of libstd, String and Vec in Rust are already restricted to a maximum capacity of isize::MAX, so this doesn't add any further restrictions on the length.

I don’t know if any bit on the pointer is guaranteed to be available. Maybe not high bits since it seems like a userland process can get up to 3 GB on 32-bit. Not low bits since a subslice of &str can start at any byte.

emilio · 2017-06-15T21:01:00Z

src/compact_cow_str.rs

+        }
+    }
+
+    /// Convert into `String`, re-using the memory allocation it was already owned.


nit: if it was already owned.

emilio · 2017-06-15T21:02:53Z

src/compact_cow_str.rs

+        if self.is_borrowed() {
+            CompactCowStr { ..*self }
+        } else {
+            Self::from(String::from(&**self).into_boxed_str())


Why can't this create a borrowed version when cloning, given it has the same lifetime?

err, nevermind, I think I see why.

If self is borrowed, the new value will be borrowed too. If not, the lifetime parameter is irrelevant: it could be 'static, and the owned memory does not necessarily live that long.

emilio · 2017-06-15T21:09:02Z

Tests need an update, and feel free to wait for @mbrubeck to review too, in case he has any other comment :)

mbrubeck

Looks good. Just a few more minor comments:

mbrubeck · 2017-06-15T23:02:44Z

src/compact_cow_str.rs

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


At least in the current implementation of libstd, String and Vec in Rust are already restricted to a maximum capacity of isize::MAX, so this doesn't add any further restrictions on the length.

mbrubeck · 2017-06-15T23:04:04Z

src/compact_cow_str.rs

+const MAX_LEN: usize = !0 >> 1;
+
+// Only the highest bit
+const TAG: usize = MAX_LEN + 1;


Reading this code, I have trouble remembering whether the tag bit is set for borrowed or owned strings. Perhaps it should be renamed to OWNED or similar.

mbrubeck · 2017-06-15T23:05:36Z

src/compact_cow_str.rs

+}
+
+impl<'a> CompactCowStr<'a> {
+    /// Wether this string refers to borrowed memory


Typo: "Whether"

SimonSapin · 2017-06-16T00:39:43Z

servo/servo#17345 (comment) has lots of failures, let’s not land this until we figure that out.

… to require less alignment and reduce the size of Token

* Avoid using some missing From impls * Manually re-order fields

Upgrade cssparser to 0.15 Depends on servo/rust-cssparser#159  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17345)

SimonSapin · 2017-06-16T11:37:38Z

@emilio, @mbrubeck, I got a green Servo try run, after fixing another double-free. (Missing mem::forget in a method that consumes self.) I’ll take your respective "Looks good" above as "r+" so that I can publish to make it easier to do a Gecko try run. Let me know if there is something else you’d like to fix and I’ll follow up.

@bors-servo r=emilio,mbrubeck

bors-servo · 2017-06-16T11:37:39Z

📌 Commit 60b953e has been approved by emilio,mbrubeck

bors-servo · 2017-06-16T11:37:41Z

⌛ Testing commit 60b953e with merge ad98a45...

Reduce the size of Token Hopefully improving parsing performance: https://bugzilla.mozilla.org/show_bug.cgi?id=1347408  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-cssparser/159)

SimonSapin · 2017-06-16T11:41:36Z

Rebase at the same base that I had been testing.

@bors-servo r=emilio,mbrubeck

bors-servo · 2017-06-16T11:41:37Z

📌 Commit 2033c23 has been approved by emilio,mbrubeck

bors-servo · 2017-06-16T11:41:39Z

⌛ Testing commit 2033c23 with merge 13cce38...

Reduce the size of Token Hopefully improving parsing performance: https://bugzilla.mozilla.org/show_bug.cgi?id=1347408  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-cssparser/159)

bors-servo · 2017-06-16T11:44:49Z

☀️ Test successful - status-travis
Approved by: emilio,mbrubeck
Pushing 13cce38 to master...

Upgrade cssparser to 0.15 Depends on servo/rust-cssparser#159  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17345)

…up); r=<try> Depends on servo/rust-cssparser#159 Source-Repo: https://github.com/servo/servo Source-Revision: 75876a0e2220b7b95541feef4393288b195b090e

…up); r=<try> Depends on servo/rust-cssparser#159 Source-Repo: https://github.com/servo/servo Source-Revision: 75876a0e2220b7b95541feef4393288b195b090e --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : 3adf5873dad10451b7d57cc1a5feec40cabeb7d7

…up); r=<try> Depends on servo/rust-cssparser#159 Source-Repo: https://github.com/servo/servo Source-Revision: 75876a0e2220b7b95541feef4393288b195b090e

SimonSapin requested a review from mbrubeck June 15, 2017 19:58

SimonSapin mentioned this pull request Jun 15, 2017

Upgrade cssparser to 0.15 servo/servo#17345

Merged

bors-servo pushed a commit to servo/servo that referenced this pull request Jun 15, 2017

Auto merge of #17345 - servo:cssparserup, r=<try>

7129e4e

Upgrade cssparser to 0.15 Depends on servo/rust-cssparser#159

emilio approved these changes Jun 15, 2017

View reviewed changes

mbrubeck approved these changes Jun 15, 2017

View reviewed changes

SimonSapin force-pushed the size_of branch from 8db65bb to 60b953e Compare June 16, 2017 00:21

SimonSapin added 8 commits June 16, 2017 10:51

Add size_of tests

b088ff6

Add CompactCowStr

5daadfd

Use CompactCowStr instead of Cow<str>

5cf34eb

Inline NumericValue struct into Token::Dimension enum fields…

3bca44d

… to require less alignment and reduce the size of Token

Backport to Rust 1.15

d6d98d3

* Avoid using some missing From impls * Manually re-order fields

Inline fields of Token::Number to align with Token::Dimension

2733bb3

Inline fields of Token::Percentage to align with Number and Dimension

e55868a

Add CompactCowStr::into_owned(self) -> String, to mimic Cow<str>.

54045c7

0.15.0

2033c23

SimonSapin force-pushed the size_of branch from 60b953e to 2033c23 Compare June 16, 2017 11:40

bors-servo merged commit 2033c23 into master Jun 16, 2017

bors-servo mentioned this pull request Jun 16, 2017

RFC: Use Rust's string-to-number conversions, instead of home-grown ones #156

Closed

SimonSapin deleted the size_of branch June 16, 2017 11:48

SimonSapin mentioned this pull request Jun 25, 2017

Make it fast! #164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce the size of Token #159

Reduce the size of Token #159

SimonSapin commented Jun 15, 2017 •

edited by larsbergstrom

Loading

emilio left a comment

emilio Jun 15, 2017

SimonSapin Jun 16, 2017

emilio Jun 15, 2017

mbrubeck Jun 15, 2017

SimonSapin Jun 16, 2017

emilio Jun 15, 2017

SimonSapin Jun 16, 2017

emilio Jun 15, 2017

emilio Jun 15, 2017

SimonSapin Jun 16, 2017

emilio commented Jun 15, 2017

mbrubeck left a comment

mbrubeck Jun 15, 2017

mbrubeck Jun 15, 2017 •

edited

Loading

SimonSapin Jun 16, 2017

mbrubeck Jun 15, 2017

SimonSapin Jun 16, 2017

SimonSapin commented Jun 16, 2017

SimonSapin commented Jun 16, 2017

bors-servo commented Jun 16, 2017

bors-servo commented Jun 16, 2017

SimonSapin commented Jun 16, 2017

bors-servo commented Jun 16, 2017

bors-servo commented Jun 16, 2017

bors-servo commented Jun 16, 2017

Reduce the size of Token #159

Reduce the size of Token #159

Conversation

SimonSapin commented Jun 15, 2017 • edited by larsbergstrom 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

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 commented Jun 15, 2017

mbrubeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrubeck Jun 15, 2017 • edited 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

SimonSapin commented Jun 16, 2017

SimonSapin commented Jun 16, 2017

bors-servo commented Jun 16, 2017

bors-servo commented Jun 16, 2017

SimonSapin commented Jun 16, 2017

bors-servo commented Jun 16, 2017

bors-servo commented Jun 16, 2017

bors-servo commented Jun 16, 2017

SimonSapin commented Jun 15, 2017 •

edited by larsbergstrom

Loading

mbrubeck Jun 15, 2017 •

edited

Loading