Skip to content

/ servo

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.

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

Optimize `TextRun::advance_for_range`. #8990

Merged
merged 3 commits into from Dec 17, 2015
Merged

Optimize `TextRun::advance_for_range`. #8990

merged 3 commits into from Dec 17, 2015

Conversation

@pcwalton
Copy link
Contributor

pcwalton commented Dec 15, 2015

The combined effects of these optimizations move advance_for_range from #1 in the layout profile on all sites I tested to #2, #3, or #4, depending on the site.

r? @mbrubeck

Review on Reviewable
pcwalton added 2 commits Dec 15, 2015
@pcwalton
util: Inline trivial range constructors.
6f2aa54 
These were showing up in the profile.
@pcwalton
gfx: Cache the total advance of each glyph store.
85c73de 
The total advance is asked for over and over.
@samlh
Copy link
Contributor

samlh commented Dec 15, 2015

Is a text run being freed and a new one being created at the same location an issue here? Or is it unlikely to cause a problem in practice?
@pcwalton pcwalton force-pushed the pcwalton:advance-for-range-optzns branch from d8ca19c to 1902929 Dec 15, 2015
@pcwalton
Copy link
Contributor Author

pcwalton commented Dec 15, 2015

@samlh Added a mitigation for this (always safe because TextRuns are always Arc'd).
@mbrubeck
Copy link
Contributor

mbrubeck commented Dec 15, 2015

-S-awaiting-review +S-fails-tidy

r=mbrubeck with the tidy failure fixed

./components/gfx/text/text_run.rs:18: trailing whitespace

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from the review on Reviewable.io
@pcwalton pcwalton force-pushed the pcwalton:advance-for-range-optzns branch from 1902929 to 520c561 Dec 16, 2015
@pcwalton
gfx: Cache the last result of
Loading status checks…
520c561 
`TextRun::index_of_first_glyph_run_containing` in TLS.

This achieves a 40% or so hit rate on Wikipedia.
@pcwalton
Copy link
Contributor Author

pcwalton commented Dec 16, 2015

@bors-servo: r=mbrubeck
@bors-servo
Copy link
Contributor

bors-servo commented Dec 16, 2015

📌 Commit 520c561 has been approved by mbrubeck
@bors-servo
Copy link
Contributor

bors-servo commented Dec 17, 2015

⌛ Testing commit 520c561 with merge 67c3cb3...
bors-servo added a commit that referenced this pull request Dec 17, 2015
@bors-servo
Auto merge of #8990 - pcwalton:advance-for-range-optzns, r=mbrubeck
Loading status checks…
67c3cb3 
Optimize `TextRun::advance_for_range`.

The combined effects of these optimizations move `advance_for_range` from #1 in the layout profile on all sites I tested to #2, #3, or #4, depending on the site.

r? @mbrubeck

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8990)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 17, 2015

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 520c561 into servo:master Dec 17, 2015
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable Review in progress: 2 of 3 files reviewed, all discussions resolved
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@frewsxcv
Copy link
Member

frewsxcv commented Dec 23, 2015

This pull request apparently caused a regression: #9057
@mbrubeck mbrubeck mentioned this pull request May 24, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mbrubeck mbrubeck

Labels
S-awaiting-merge S-fails-tidy
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@pcwalton @samlh @mbrubeck @bors-servo @frewsxcv @highfive
You can’t perform that action at this time.