Use the whitespace flag in each glyph instead of going to the string for whitespace stripping. #8994

pcwalton · 2015-12-16T00:04:58Z

str::slice_chars was O(n) and #2 in the profile.

r? @mbrubeck

highfive · 2015-12-16T00:05:04Z

Warning

These commits modify layout code, but no reftests are modified. Please consider adding a reftest!

Ms2ger · 2015-12-16T11:17:17Z

Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions.

components/gfx/text/text_run.rs, line 146 [r1] (raw file):
Or self.prev_glyph_run.take()

components/gfx/text/text_run.rs, line 176 [r1] (raw file):
take()

components/gfx/text/text_run.rs, line 181 [r1] (raw file):
Rm

Comments from the review on Reviewable.io

pcwalton · 2015-12-16T17:49:38Z

Comments addressed.


        Use the whitespace flag in each glyph instead of going to the string for

whitespace stripping. `str::slice_chars` was O(n) and #2 in the profile.

mbrubeck · 2015-12-16T17:58:39Z

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from the review on Reviewable.io

mbrubeck · 2015-12-16T17:58:55Z

@bors-servo r+

bors-servo · 2015-12-16T17:58:56Z

📌 Commit ee7f2de has been approved by mbrubeck

bors-servo · 2015-12-17T04:51:59Z

⌛ Testing commit ee7f2de with merge 7665a01...


        Auto merge of #8994 - pcwalton:whitespace-stripping-optzns, r=mbrubeck

@mbrubeck

Use the whitespace flag in each glyph instead of going to the string for whitespace stripping. `str::slice_chars` was O(n) and #2 in the profile. r? @mbrubeck  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8994)

bors-servo · 2015-12-17T05:21:44Z

💔 Test failed - mac-rel-css

jdm · 2015-12-17T13:53:58Z

@pcwalton, can you reproduce these locally?

nox · 2015-12-30T19:01:30Z

@bors-servo try

nox · 2015-12-30T19:01:47Z

@bors-servo clean try

nox · 2015-12-30T19:02:01Z

@bors-servo try force

nox · 2015-12-30T19:02:52Z

@bors-servo clean try force

nox · 2015-12-30T19:05:50Z

@bors-servo try

nox · 2015-12-30T19:05:57Z

@bors-servo r- force

nox · 2015-12-30T19:06:03Z

@bors-servo try

larsbergstrom · 2015-12-31T20:29:12Z

@bors-servo r=mbrubeck

bors-servo · 2015-12-31T20:29:12Z

📌 Commit ee7f2de has been approved by mbrubeck

frewsxcv · 2016-01-18T03:38:29Z

@bors-servo r=mbrubeck

bors-servo · 2016-01-18T03:38:29Z

📌 Commit ee7f2de has been approved by mbrubeck

nox · 2016-02-02T22:10:59Z

@bors-servo r=mbrubeck

bors-servo · 2016-02-02T22:11:00Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
There's another pull request that is currently being tested, blocking this pull request: #9492

bors-servo · 2016-02-02T22:11:00Z

📌 Commit ee7f2de has been approved by mbrubeck

nox · 2016-02-02T22:11:34Z

@bors-servo retry

bors-servo · 2016-02-02T22:39:15Z

⌛ Testing commit ee7f2de with merge de20299...


        Auto merge of #8994 - pcwalton:whitespace-stripping-optzns, r=mbrubeck

@mbrubeck

Use the whitespace flag in each glyph instead of going to the string for whitespace stripping. `str::slice_chars` was O(n) and #2 in the profile. r? @mbrubeck  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8994)

bors-servo · 2016-02-02T23:26:21Z

💔 Test failed - linux-rel

jdm · 2016-02-02T23:54:22Z

  ▶ FAIL [expected PASS] /css21_dev/html4/block-non-replaced-width-007.htm
  └   → /css21_dev/html4/block-non-replaced-width-007.htm eb195905e02d7943de70d2b8ac617e074179e996
/css21_dev/html4/reference/block-non-replaced-width-007-ref.htm 47ad8313feacfb8add7c5d1c014c7975027bf5b7
Testing eb195905e02d7943de70d2b8ac617e074179e996 == 47ad8313feacfb8add7c5d1c014c7975027bf5b7

  ▶ PASS [expected FAIL] /css21_dev/html4/floats-wrap-bfc-002-right-table.htm

jdm · 2016-02-26T15:58:01Z

@pcwalton This had test failures. Are you planning to investigate them, or should we close the PR?

pcwalton · 2016-02-29T22:23:00Z

I'll look at them.

jdm · 2016-03-10T19:14:40Z

Filed #9955 to keep track of this. Feel free to reopen it when you end up making progress.

highfive added the S-awaiting-review label Dec 16, 2015

mbrubeck self-assigned this Dec 16, 2015

pcwalton force-pushed the pcwalton:whitespace-stripping-optzns branch from 6614aef to ee7f2de Dec 16, 2015

Use the whitespace flag in each glyph instead of going to the string for

Loading status checks…

ee7f2de

whitespace stripping. `str::slice_chars` was O(n) and #2 in the profile.

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 16, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 17, 2015

nox closed this Dec 30, 2015

nox reopened this Dec 30, 2015

highfive removed the S-tests-failed label Dec 31, 2015

highfive added the S-awaiting-merge label Dec 31, 2015

frewsxcv closed this Jan 18, 2016

frewsxcv reopened this Jan 18, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 2, 2016

jdm mentioned this pull request Mar 10, 2016

Use the whitespace flag in each glyph instead of going to the string for whitespace stripping #9955

Open

jdm closed this Mar 10, 2016

servo / servo

Use the whitespace flag in each glyph instead of going to the string for whitespace stripping. #8994

Use the whitespace flag in each glyph instead of going to the string for whitespace stripping. #8994

pcwalton commented Dec 16, 2015

highfive commented Dec 16, 2015

Ms2ger commented Dec 16, 2015

pcwalton commented Dec 16, 2015

mbrubeck commented Dec 16, 2015

mbrubeck commented Dec 16, 2015

bors-servo commented Dec 16, 2015

bors-servo commented Dec 17, 2015

bors-servo commented Dec 17, 2015

jdm commented Dec 17, 2015

nox commented Dec 30, 2015

nox commented Dec 30, 2015

nox commented Dec 30, 2015

nox commented Dec 30, 2015

nox commented Dec 30, 2015

nox commented Dec 30, 2015

nox commented Dec 30, 2015

larsbergstrom commented Dec 31, 2015

bors-servo commented Dec 31, 2015

frewsxcv commented Jan 18, 2016

bors-servo commented Jan 18, 2016

nox commented Feb 2, 2016

bors-servo commented Feb 2, 2016

bors-servo commented Feb 2, 2016

nox commented Feb 2, 2016

bors-servo commented Feb 2, 2016

bors-servo commented Feb 2, 2016

jdm commented Feb 2, 2016

jdm commented Feb 26, 2016

pcwalton commented Feb 29, 2016

jdm commented Mar 10, 2016

servo / servo

Join GitHub today

Use the whitespace flag in each glyph instead of going to the string for whitespace stripping. #8994

Use the whitespace flag in each glyph instead of going to the string for whitespace stripping. #8994

Conversation

pcwalton commented Dec 16, 2015

highfive commented Dec 16, 2015

Ms2ger commented Dec 16, 2015

pcwalton commented Dec 16, 2015

mbrubeck commented Dec 16, 2015

mbrubeck commented Dec 16, 2015

bors-servo commented Dec 16, 2015

bors-servo commented Dec 17, 2015

bors-servo commented Dec 17, 2015

jdm commented Dec 17, 2015

nox commented Dec 30, 2015

nox commented Dec 30, 2015

nox commented Dec 30, 2015

nox commented Dec 30, 2015

nox commented Dec 30, 2015

nox commented Dec 30, 2015

nox commented Dec 30, 2015

larsbergstrom commented Dec 31, 2015

bors-servo commented Dec 31, 2015

frewsxcv commented Jan 18, 2016

bors-servo commented Jan 18, 2016

nox commented Feb 2, 2016

bors-servo commented Feb 2, 2016

bors-servo commented Feb 2, 2016

nox commented Feb 2, 2016

bors-servo commented Feb 2, 2016

bors-servo commented Feb 2, 2016

jdm commented Feb 2, 2016

jdm commented Feb 26, 2016

pcwalton commented Feb 29, 2016

jdm commented Mar 10, 2016