Skip to content
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

Coalescing optimization #1271

Merged
merged 4 commits into from May 22, 2017
Merged

Coalescing optimization #1271

merged 4 commits into from May 22, 2017

Conversation

@kvark
Copy link
Member

kvark commented May 18, 2017

Fixes #1266
(if it doesn't fix it yet, the PR would be a step in the right direction anyway)

This PR removes indexing and dynamic allocations from TexturePage::coalesce, cleans up the code, adds a basic benchmark for it. Results on my machine - time dropped by about 25%. It's not super great, but I believe that real-world parameters have improved more, given that we are able to capture intermediate coalescing results now even when timed out.

r? @glennw


This change is Reviewable

free_list[work_index].union(&free_list[candidate_index]);
free_list[candidate_index].size.width = 0
}
new_free_list.push(free_list[work_index])

This comment has been minimized.

@kvark

kvark May 18, 2017

Author Member

this line looks like a bug to me (adding a work item during the candidates iteration), and I removed it

@glennw
Copy link
Member

glennw commented May 18, 2017

@pcwalton Would you mind taking a look at this one, if you can still recall how the coalesce code works? :)

@kvark kvark changed the title Coalescing optimiazation Coalescing optimization May 19, 2017
@glennw
Copy link
Member

glennw commented May 22, 2017

I had a look over this and it seems right to me. Also ran the Servo tests and tried a few sites manually.

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented May 22, 2017

📌 Commit 031419d has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented May 22, 2017

Testing commit 031419d with merge ecd8416...

bors-servo added a commit that referenced this pull request May 22, 2017
Coalescing optimization

Fixes #1266
(if it doesn't fix it yet, the PR would be a step in the right direction anyway)

This PR removes indexing and dynamic allocations from `TexturePage::coalesce`, cleans up the code, adds a basic benchmark for it. Results on my machine - time dropped by about 25%. It's not super great, but I believe that real-world parameters have improved more, given that we are able to capture intermediate coalescing results now even when timed out.

r? @glennw

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1271)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 22, 2017

☀️ Test successful - status-travis
Approved by: glennw
Pushing ecd8416 to master...

@bors-servo bors-servo merged commit 031419d into servo:master May 22, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@staktrace
Copy link
Contributor

staktrace commented May 23, 2017

@kvark, This PR seems to hit an assertion failure (or something else causing a debug-only panic) when running the gecko reftest layout/reftests/bugs/1081185-1.html. The panic happens in texture_cache::FreeListBin::for_size, see full backtrace.

@kvark kvark deleted the kvark:tex-cache branch May 23, 2017
@kvark
Copy link
Member Author

kvark commented May 23, 2017

@staktrace sorry about that! looking now

bors-servo added a commit that referenced this pull request May 23, 2017
Support for allocating zero sized textures

Addresses #1271 (comment)
cc @staktrace
r? @glennw

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1287)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.