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

Extend the scope of render target allocation strategy acrosss layers #3374

Merged
merged 7 commits into from
Nov 30, 2018

Conversation

kvark
Copy link
Member

@kvark kvark commented Nov 29, 2018

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1509672

Technically consists of 3 parts:

  • cleanup bits, don't affect performance
  • spread the dynamic target allocation bins across the layers. Previously, we had each layer with it's own allocator, and we were only ever trying to allocate from the last one. This scheme was weak against cases where we reach the ideal maximum target size, since guillotine allocation of small bits forced us to spawn more and more layers... reaching almost 150 in the target case. New scheme uses the layers more efficiently, reducing the layers to just 3 (.. 50x reduction :P). Pending try push.
  • when we reach the ideal max size, also round up the requested size to 256. This makes allocations within a layer to be more robust against small inputs. With this change, the number of layers drops to just 2 (.. 75x reduction lol), which appears to be minimal for this case. Pending try push.

The page scrolls smoothly with those changes, on my GTX TI 1050 at least.

r? @gw3583

Note: the bugzilla issue also suffers from poor batching, I'm going to look at it separately.


This change is Reviewable

@kvark
Copy link
Member Author

kvark commented Nov 29, 2018

Both tries look green. Appveyor used an older Rust version, which is updated in the last commit.
OSX Release TC bots are being updated to 1.30 by @staktrace as we speak.
This is ready for reviews!

Copy link
Contributor

@gw3583 gw3583 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@gw3583
Copy link
Contributor

gw3583 commented Nov 29, 2018

Seems sane! Do we need to do any profiling / testing before merging?

@kvark
Copy link
Member Author

kvark commented Nov 30, 2018

@gw3583 I visited a few websites with RT debug display enabled, and the improvement is real. On cnn.com we are down from 3 large (~3K x 2K) slices to just one. On HN, similarly, we only have a single slice with everything instead of 3.

Looks like this is going to be a major win on common sites, not just extreme cases. 🎉

@gw3583
Copy link
Contributor

gw3583 commented Nov 30, 2018

@kvark Cool, ship it! 🚀

@staktrace
Copy link
Contributor

The mac builders should have 1.30 now

@zptan
Copy link

zptan commented Nov 30, 2018

OS X debug tests failed:

...
sccache --stop-server || true
Stopping sccache server...
error: couldn't connect to server
caused by: Connection refused (os error 61)
mkdir -p ../artifacts
RUST_LOG=sccache=trace SCCACHE_ERROR_LOG=$PWD/../artifacts/sccache.log sccache --start-server
TRACE:sccache::cmdline: parse
TRACE:sccache::commands: Command::StartServer
Starting sccache server...
TRACE:sccache::commands: run_server_process
TRACE:sccache::cmdline: parse
TRACE:sccache::commands: Command::InternalStartServer
error: Timed out waiting for server startup
...

@kvark
Copy link
Member Author

kvark commented Nov 30, 2018

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit c2f04cd with merge d99dedb...

bors-servo pushed a commit that referenced this pull request Nov 30, 2018
Extend the scope of render target allocation strategy acrosss layers

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1509672

Technically consists of 3 parts:
  - cleanup bits, don't affect performance
  - spread the dynamic target allocation bins across the layers. Previously, we had each layer with it's own allocator, and we were only ever trying to allocate from the last one. This scheme was weak against cases where we reach the ideal maximum target size, since guillotine allocation of small bits forced us to spawn more and more layers... reaching almost 150 in the target case. New scheme uses the layers more efficiently, reducing the layers to just 3 (.. 50x reduction :P). Pending [try push](https://treeherder.mozilla.org/#/jobs?repo=try&revision=27464c7d62f0f05a0bc96a7133b55e9706d3d449).
  - when we reach the ideal max size, also round up the requested size to 256. This makes allocations *within a layer* to be more robust against small inputs. With this change, the number of layers drops to just 2 (.. 75x reduction lol), which appears to be minimal for this case. Pending [try push](https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f4593fd68455842a7b12f396d0abbdf887a11a0).

The page scrolls smoothly with those changes, on my GTX TI 1050 at least.

r? @gw3583

Note: the bugzilla issue also suffers from poor batching, I'm going to look at it separately.

<!-- 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/3374)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-appveyor, status-taskcluster
State: approved= try=True

@kvark
Copy link
Member Author

kvark commented Nov 30, 2018

@gw3583 I've added a bit of code for validating texture allocator correctness, at least at test time.
@bors-servo r=gw3583

@bors-servo
Copy link
Contributor

📌 Commit 3362a5e has been approved by gw3583

@bors-servo
Copy link
Contributor

⌛ Testing commit 3362a5e with merge dbaa109...

bors-servo pushed a commit that referenced this pull request Nov 30, 2018
Extend the scope of render target allocation strategy acrosss layers

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1509672

Technically consists of 3 parts:
  - cleanup bits, don't affect performance
  - spread the dynamic target allocation bins across the layers. Previously, we had each layer with it's own allocator, and we were only ever trying to allocate from the last one. This scheme was weak against cases where we reach the ideal maximum target size, since guillotine allocation of small bits forced us to spawn more and more layers... reaching almost 150 in the target case. New scheme uses the layers more efficiently, reducing the layers to just 3 (.. 50x reduction :P). Pending [try push](https://treeherder.mozilla.org/#/jobs?repo=try&revision=27464c7d62f0f05a0bc96a7133b55e9706d3d449).
  - when we reach the ideal max size, also round up the requested size to 256. This makes allocations *within a layer* to be more robust against small inputs. With this change, the number of layers drops to just 2 (.. 75x reduction lol), which appears to be minimal for this case. Pending [try push](https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f4593fd68455842a7b12f396d0abbdf887a11a0).

The page scrolls smoothly with those changes, on my GTX TI 1050 at least.

r? @gw3583

Note: the bugzilla issue also suffers from poor batching, I'm going to look at it separately.

<!-- 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/3374)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: gw3583
Pushing dbaa109 to master...

@bors-servo bors-servo merged commit 3362a5e into servo:master Nov 30, 2018
@heftig
Copy link

heftig commented Nov 30, 2018

With the latter try build the issue is gone for me. Thanks! 👍

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 2, 2018
…ab3e7ace77d6 (WR PR #3374). r=kats

servo/webrender#3374

Differential Revision: https://phabricator.services.mozilla.com/D13626

--HG--
extra : moz-landing-system : lando
mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 2, 2018
@kvark kvark deleted the rt-alloc branch December 12, 2018 01:48
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…ab3e7ace77d6 (WR PR #3374). r=kats

servo/webrender#3374

Differential Revision: https://phabricator.services.mozilla.com/D13626

UltraBlame original commit: be65d09ef059e78daf60df9737503cfcb6dc8a86
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
…ab3e7ace77d6 (WR PR #3374). r=kats

servo/webrender#3374

Differential Revision: https://phabricator.services.mozilla.com/D13626

UltraBlame original commit: be65d09ef059e78daf60df9737503cfcb6dc8a86
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
…ab3e7ace77d6 (WR PR #3374). r=kats

servo/webrender#3374

Differential Revision: https://phabricator.services.mozilla.com/D13626

UltraBlame original commit: be65d09ef059e78daf60df9737503cfcb6dc8a86
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants