Make match_ignore_ascii_case more efficient, add ascii_case_insensitive_phf_map #122

SimonSapin · 2017-02-25T13:25:44Z

This improves the performance of match_ignore_ascii_case! by replacing calls to str::eq_ignore_ascii_case with plain string equality. The input string is converted to ASCII lower-case once, using a stack allocated buffer. A proc_macro_derive is used internally to ensure that string patterns are already lower-case, and to computed the size of the buffer (the length of the longest pattern).

This should improve runtime performance, it the amount of generated MIR or LLVM IR in release mode still looks proportional to the number of patterns, so this by itself won’t help much with the code bloat in parse_color_keyword.

To deal with that, this PR also adds a ascii_case_insensitive_phf_map! macro that reuses the same stack-allocated buffer mechanism to lower-case an input string, and combines it with a phf static hash map.

The two macros are similar but make different treadoffs. PHF probably generates faster and less bloated code, but the map’s values need to be given as a string that contains Rust syntax, due to limitations of procedural macros in current stable Rust. On the other hand, generating a match expression allows using control flow statements like return or continue in its match arms.

Fixes #115.

This change is

Previously, the compiler would emit many `eq_ignore_ascii_case` calls, leading to code bloat and probably some slowness. Now, we pre-lowercase the input in a stack-allocated buffer then match exact strings. Hopefully, the optimizer can turn this into a static table and a loop.

Fix #115.

Let users pass a `&foo` borrow. `&Cow<str>` can auto-deref to `&str`.

…f_map! We can do the lower-casing at compile time in a proc-macro. Note that `match_ignore_ascii_case!` still has that requirement, since it’s `macro_rules!` that generates a `match` expression.

* String patterns now need to be already ASCII lower-case * Input string is no longer implicitly borrowed, add `&` as needed.

SimonSapin · 2017-02-25T13:26:25Z

r? @Manishearth, CC @nox

~~Do not merge just yet, I’ll make sure Servo can build with this first :)~~ Done.

Update cssparser to 0.11  Depends on servo/rust-cssparser#122. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____

Manishearth

r=me looks good. Can't wait for proper proc macro support.

SimonSapin · 2017-02-25T18:51:53Z

@bors-servo r=Manishearth

bors-servo · 2017-02-25T18:51:54Z

📌 Commit 136c97a has been approved by Manishearth

bors-servo · 2017-02-25T18:51:57Z

⌛ Testing commit 136c97a with merge ded474a...

Make match_ignore_ascii_case more efficient, add ascii_case_insensitive_phf_map This improves the performance of `match_ignore_ascii_case!` by replacing calls to `str::eq_ignore_ascii_case` with plain string equality. The input string is converted to ASCII lower-case once, using a stack allocated buffer. A `proc_macro_derive` is used internally to ensure that string patterns are already lower-case, and to computed the size of the buffer (the length of the longest pattern). This should improve runtime performance, it the amount of generated MIR or LLVM IR in release mode still looks proportional to the number of patterns, so this by itself won’t help much with the code bloat in `parse_color_keyword`. To deal with that, this PR also adds a `ascii_case_insensitive_phf_map!` macro that reuses the same stack-allocated buffer mechanism to lower-case an input string, and combines it with a [`phf`](https://github.com/sfackler/rust-phf) static hash map. The two macros are similar but make different treadoffs. PHF probably generates faster and less bloated code, but the map’s values need to be given as a string that contains Rust syntax, due to limitations of procedural macros in current stable Rust. On the other hand, generating a `match` expression allows using control flow statements like `return` or `continue` in its match arms. Fixes #115.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-cssparser/122)

bors-servo · 2017-02-25T19:02:36Z

💔 Test failed - status-travis

SimonSapin · 2017-02-25T19:22:15Z

@bors-servo retry

Job timed out.

bors-servo · 2017-02-25T19:22:19Z

⌛ Testing commit 136c97a with merge 89115eb...

Make match_ignore_ascii_case more efficient, add ascii_case_insensitive_phf_map This improves the performance of `match_ignore_ascii_case!` by replacing calls to `str::eq_ignore_ascii_case` with plain string equality. The input string is converted to ASCII lower-case once, using a stack allocated buffer. A `proc_macro_derive` is used internally to ensure that string patterns are already lower-case, and to computed the size of the buffer (the length of the longest pattern). This should improve runtime performance, it the amount of generated MIR or LLVM IR in release mode still looks proportional to the number of patterns, so this by itself won’t help much with the code bloat in `parse_color_keyword`. To deal with that, this PR also adds a `ascii_case_insensitive_phf_map!` macro that reuses the same stack-allocated buffer mechanism to lower-case an input string, and combines it with a [`phf`](https://github.com/sfackler/rust-phf) static hash map. The two macros are similar but make different treadoffs. PHF probably generates faster and less bloated code, but the map’s values need to be given as a string that contains Rust syntax, due to limitations of procedural macros in current stable Rust. On the other hand, generating a `match` expression allows using control flow statements like `return` or `continue` in its match arms. Fixes #115.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-cssparser/122)

SimonSapin · 2017-02-25T19:35:52Z

Not sure what homu is doing, https://travis-ci.org/servo/rust-cssparser/builds/205343338 is green.

Update cssparser to 0.11  <s>Depends on https://github.com/servo/rust-cssparser/pull/122.</s> ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15735)

…p); r=emilio  <s>Depends on https://github.com/servo/rust-cssparser/pull/122.</s> ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____  Source-Repo: https://github.com/servo/servo Source-Revision: 94e563e4d9292d7b19ce061e070cda358e822172 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : b6f18aa92334574e0874e2ce8505c03f295c7d49

…p); r=emilio  <s>Depends on https://github.com/servo/rust-cssparser/pull/122.</s> ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____  Source-Repo: https://github.com/servo/servo Source-Revision: 94e563e4d9292d7b19ce061e070cda358e822172

…p); r=emilio  <s>Depends on https://github.com/servo/rust-cssparser/pull/122.</s> ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____  Source-Repo: https://github.com/servo/servo Source-Revision: 94e563e4d9292d7b19ce061e070cda358e822172 UltraBlame original commit: 6fec46f1729153a0e230f4dac07ba776eebe6102

SimonSapin added 9 commits February 24, 2017 21:59

Update syn and encoding_rs

63c2044

Use docs.rs for docs.

0592bea

Add a ascii_case_insensitive_phf_map macro, use it for color keywords.

0192030

Fix #115.

Add comments to explain new unsafe code.

5a17750

Don’t borrow $input in match_ignore_ascii_case!

ea6843d

Let users pass a `&foo` borrow. `&Cow<str>` can auto-deref to `&str`.

Document macros.

983529f

Remove the already-lowercase requirement in ascii_case_insensitive_ph…

b0c427d

…f_map! We can do the lower-casing at compile time in a proc-macro. Note that `match_ignore_ascii_case!` still has that requirement, since it’s `macro_rules!` that generates a `match` expression.

Increment version for breaking changes in match_ignore_ascii_case

78996fc

* String patterns now need to be already ASCII lower-case * Input string is no longer implicitly borrowed, add `&` as needed.

SimonSapin added 4 commits February 25, 2017 15:41

Rename internal things per what they do.

0db4f8e

Be more robust regarding attribute ordering in proc macros.

c85990a

Support macros being used in a module with #[deny(unsafe_code)]

df6892b

match_ignore_ascii_case: extend lifetime of input.

136c97a

SimonSapin mentioned this pull request Feb 25, 2017

Update cssparser to 0.11 servo/servo#15735

Merged

5 tasks

Manishearth approved these changes Feb 25, 2017

View reviewed changes

Remove an obsolete bit of comment.

138ccaf

SimonSapin merged commit 4890542 into master Feb 25, 2017

SimonSapin deleted the macros branch February 25, 2017 19:36

nox mentioned this pull request Feb 27, 2017

Update syn to 0.11 servo/servo#15542

Closed

4 tasks

SimonSapin mentioned this pull request Feb 28, 2017

Rewrite procedural macro based on the enum discriminant trick #123

Merged

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

Make match_ignore_ascii_case more efficient, add ascii_case_insensitive_phf_map #122

Make match_ignore_ascii_case more efficient, add ascii_case_insensitive_phf_map #122

SimonSapin commented Feb 25, 2017 •

edited

Loading

SimonSapin commented Feb 25, 2017 •

edited

Loading

Manishearth left a comment

SimonSapin commented Feb 25, 2017

bors-servo commented Feb 25, 2017

bors-servo commented Feb 25, 2017

bors-servo commented Feb 25, 2017

SimonSapin commented Feb 25, 2017

bors-servo commented Feb 25, 2017

SimonSapin commented Feb 25, 2017

Make match_ignore_ascii_case more efficient, add ascii_case_insensitive_phf_map #122

Make match_ignore_ascii_case more efficient, add ascii_case_insensitive_phf_map #122

Conversation

SimonSapin commented Feb 25, 2017 • edited Loading

SimonSapin commented Feb 25, 2017 • edited Loading

Manishearth left a comment

Choose a reason for hiding this comment

SimonSapin commented Feb 25, 2017

bors-servo commented Feb 25, 2017

bors-servo commented Feb 25, 2017

bors-servo commented Feb 25, 2017

SimonSapin commented Feb 25, 2017

bors-servo commented Feb 25, 2017

SimonSapin commented Feb 25, 2017

SimonSapin commented Feb 25, 2017 •

edited

Loading

SimonSapin commented Feb 25, 2017 •

edited

Loading