border: Don't always share instances across borders if the instances depend on the available border size. #3067

emilio · 2018-09-17T16:20:18Z

I've added a big comment to BorderRenderTaskInfo with my current understanding
of the invariants this code is trying to preserve.

Please sanity-check it, but I think this patch should both be sound and also
prevent the scrolling-franzine issue
(https://bugzilla.mozilla.org/show_bug.cgi?id=1489644).

Fixes #3058

This change is

emilio · 2018-09-17T16:27:53Z

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7214c7ebbc3611712383bf1bf982e5304f1019b

kvark · 2018-09-17T16:30:00Z

webrender/src/border.rs

@@ -528,10 +528,63 @@ pub struct BorderSegmentInfo {
    widths: DeviceSize,
 }

+bitflags! {
+    /// Whether we depend in the available size for the border (effectively in


did you mean "depend on" instead of "depend in"?

Yup, will fix.

kvark · 2018-09-17T16:30:00Z

webrender/src/border.rs

+        }
+
+        let device_size = (*local_size * scale).to_i32();
+        if self.available_size_dependence.intersects(AvailableSizeDependence::VERTICAL) {


nit: use contains?

kvark · 2018-09-17T16:30:00Z

webrender/src/border.rs

+    ) -> DeviceIntSize {
+        let mut size = DeviceIntSize::zero();
+        if self.available_size_dependence.is_empty() {
+            return size;


why aren't we returning the *local_size * scale here?

Because for borders where we don't depend on any size we just want the same value for all of them, so they hit the cache and we don't build instances unnecessarily.

what about a particular width/height then? shouldn't we put 0.0 into width if the key is only height-sensitive?

That's what we do.

Note that we return size instead of device_size. I'm open to suggestions about how to make that clearer.


        border: Don't always share instances across borders if the instances …

…depend on the available border size. I've added a big comment to BorderRenderTaskInfo with my current understanding of the invariants this code is trying to preserve. Please sanity-check it, but I think this patch should both be sound and also prevent the scrolling-franzine issue (https://bugzilla.mozilla.org/show_bug.cgi?id=1489644). Fixes #3058

emilio · 2018-09-17T17:11:54Z

Comments addressed.

emilio · 2018-09-17T17:12:37Z

Reftests and such look all green so far, yay :)

kvark

Thank you, I'm convinced now :)

emilio · 2018-09-17T18:38:00Z

@bors-servo r=kvark

@gw3583 feel free to take a quick skim at the changes just to sanity-check, if you want :)

bors-servo · 2018-09-17T18:38:01Z

📌 Commit 4aac021 has been approved by kvark

bors-servo · 2018-09-17T18:38:04Z

⌛ Testing commit 4aac021 with merge 5a1985a...


        Auto merge of #3067 - emilio:border-caching, r=kvark

border: Don't always share instances across borders if the instances depend on the available border size. I've added a big comment to BorderRenderTaskInfo with my current understanding of the invariants this code is trying to preserve. Please sanity-check it, but I think this patch should both be sound and also prevent the scrolling-franzine issue (https://bugzilla.mozilla.org/show_bug.cgi?id=1489644). Fixes #3058  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3067)

bors-servo · 2018-09-17T20:01:55Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 5a1985a to master...

kvark reviewed Sep 17, 2018

View changes

emilio force-pushed the emilio:border-caching branch from 7ea36a5 to 4aac021 Sep 17, 2018

kvark approved these changes Sep 17, 2018

View changes

bors-servo merged commit 4aac021 into servo:master Sep 17, 2018
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

servo / webrender

border: Don't always share instances across borders if the instances depend on the available border size. #3067

border: Don't always share instances across borders if the instances depend on the available border size. #3067

emilio commented Sep 17, 2018 •

edited by larsbergstrom

emilio commented Sep 17, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Sep 17, 2018

emilio commented Sep 17, 2018

kvark left a comment

emilio commented Sep 17, 2018

bors-servo commented Sep 17, 2018

bors-servo commented Sep 17, 2018

bors-servo commented Sep 17, 2018

servo / webrender

Join GitHub today

border: Don't always share instances across borders if the instances depend on the available border size. #3067

border: Don't always share instances across borders if the instances depend on the available border size. #3067

Conversation

emilio commented Sep 17, 2018 • edited by larsbergstrom

emilio commented Sep 17, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio Sep 17, 2018 • edited

emilio commented Sep 17, 2018

emilio commented Sep 17, 2018

kvark left a comment

emilio commented Sep 17, 2018

bors-servo commented Sep 17, 2018

bors-servo commented Sep 17, 2018

bors-servo commented Sep 17, 2018

emilio commented Sep 17, 2018 •

edited by larsbergstrom

emilio Sep 17, 2018 •

edited