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

Separate interning Image & YuvImage #3393

Merged
merged 3 commits into from Dec 18, 2018
Merged

Conversation

@djg
Copy link
Contributor

djg commented Dec 7, 2018

In support of #3385, extract Image and YUVImage. This patch is based up PR #3392 and requires that to land first.


This change is Reviewable

@gw3583
Copy link
Collaborator

gw3583 commented Dec 9, 2018

Could you rebase this to remove the patches that have landed now?

@djg djg force-pushed the djg:separate_interning_image branch from 7e094ff to 6ee310f Dec 11, 2018
@djg
Copy link
Contributor Author

djg commented Dec 11, 2018

Could you rebase this to remove the patches that have landed now?

Done

Copy link
Collaborator

gw3583 left a comment

Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @djg)


webrender/src/prim_store/mod.rs, line 2587 at r1 (raw file):

                // write_segment(yuv_image_data, *segment_instance_index, frame_state, scratch);
                debug_assert!(*segment_instance_index != SegmentInstanceIndex::INVALID);
                if *segment_instance_index != SegmentInstanceIndex::UNUSED {

Let's try to find a way to not duplicate these code blocks.


webrender/src/prim_store/mod.rs, line 2716 at r1 (raw file):

                //write_segment(image_data, image_instance.segment_instance_index, frame_state, scratch);
                if image_instance.segment_instance_index != SegmentInstanceIndex::UNUSED {

Same here

@djg djg force-pushed the djg:separate_interning_image branch from c80d042 to 986791a Dec 11, 2018
@djg
Copy link
Contributor Author

djg commented Dec 11, 2018

It appears that the osx-release taskcluster job failed to start. I've restarted it.

@gw3583
Copy link
Collaborator

gw3583 commented Dec 12, 2018

That last commit seems a bit more complex (esp. the cache fn) than I was thinking it would be, but I haven't looked in detail yet - I might just be missing something.

The general idea of the GpuCacheRequest being a by-value type is that it should work around the borrowck issues that way. I'll pull the branch today and see if I can provide some more useful comments than the vague details above, sorry!

@djg
Copy link
Contributor Author

djg commented Dec 12, 2018

That last commit seems a bit more complex (esp. the cache fn) than I was thinking it would be, but I haven't looked in detail yet - I might just be missing something.

The general idea of the GpuCacheRequest being a by-value type is that it should work around the borrowck issues that way. I'll pull the branch today and see if I can provide some more useful comments than the vague details above, sorry!

@gw3583, in the previous incarnation of the code there was a split borrow of the prim fields and the borrowck was happy. It's unhappy with:

if Some(foo) = gpu_cache.bar(&self.common.gpu_handle) {
    ...
    self.write_prim_gpu_blocks(...);
    ...
}

because it can't see that fn write_prim_gpu_blocks() doesn't access self.common.

I welcome a second set of 👀 on the code.

@gw3583
Copy link
Collaborator

gw3583 commented Dec 13, 2018

This is roughly what I was thinking [1] - does that seem like it should achieve the same outcome and work for the rest of the prim types?

[1] gw3583@a6910bc

@djg djg force-pushed the djg:separate_interning_image branch 2 times, most recently from 1bde4fb to 579ecd6 Dec 17, 2018
Copy link
Contributor Author

djg left a comment

Reviewable status: 2 of 9 files reviewed, 3 unresolved discussions (waiting on @gw3583)


webrender/src/prim_store/mod.rs, line 963 at r1 (raw file):

write_prim_gpu_blocks

webrender/src/prim_store/mod.rs, line 2587 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Let's try to find a way to not duplicate these code blocks.

Done.


webrender/src/prim_store/mod.rs, line 2716 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Same here

Done.

@djg
Copy link
Contributor Author

djg commented Dec 17, 2018

@gw3583 Are we good to proceed with these changes?

@gw3583
Copy link
Collaborator

gw3583 commented Dec 17, 2018

Yup, I think so - do we need another try / talos run or should this be good to go?

@gw3583
Copy link
Collaborator

gw3583 commented Dec 17, 2018

@bors-servo delegate+

@bors-servo
Copy link
Contributor

bors-servo commented Dec 17, 2018

✌️ @djg can now approve this pull request

@djg djg force-pushed the djg:separate_interning_image branch 2 times, most recently from 3f1d774 to 254e8df Dec 17, 2018
@bors-servo
Copy link
Contributor

bors-servo commented Dec 17, 2018

The latest upstream changes (presumably #3426) made this pull request unmergeable. Please resolve the merge conflicts.

@djg djg force-pushed the djg:separate_interning_image branch from 254e8df to 0541e3c Dec 18, 2018
@djg djg mentioned this pull request Dec 18, 2018
@gw3583
gw3583 approved these changes Dec 18, 2018
@gw3583
Copy link
Collaborator

gw3583 commented Dec 18, 2018

@bors-servo delegate+

@bors-servo
Copy link
Contributor

bors-servo commented Dec 18, 2018

✌️ @djg can now approve this pull request

@djg
Copy link
Contributor Author

djg commented Dec 18, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Dec 18, 2018

📌 Commit 0541e3c has been approved by gw

bors-servo added a commit that referenced this pull request Dec 18, 2018
Separate interning Image & YuvImage

In support of #3385, extract Image and YUVImage. This patch is based up PR #3392 and requires that to land first.

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

bors-servo commented Dec 18, 2018

Testing commit 0541e3c with merge 1126699...

@bors-servo
Copy link
Contributor

bors-servo commented Dec 18, 2018

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

@bors-servo bors-servo merged commit 0541e3c into servo:master Dec 18, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 7 files, 3 discussions left (gw3583)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
bors-servo added a commit that referenced this pull request Dec 18, 2018
Separate interning borders

In support of #3385, extract normal and image borders. This patch is based up PR #3393 and requires that to land first.

<!-- 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/3429)
<!-- Reviewable:end -->
@djg djg deleted the djg:separate_interning_image branch Dec 18, 2018
bors-servo added a commit that referenced this pull request Dec 18, 2018
Separate interning borders

In support of #3385, extract normal and image borders. This patch is based up PR #3393 and requires that to land first.

<!-- 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/3429)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 19, 2018
…27538715a2e5 (WR PR #3393). r=kats

servo/webrender#3393

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

--HG--
extra : moz-landing-system : lando
mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 19, 2018
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…27538715a2e5 (WR PR #3393). r=kats

servo/webrender#3393

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

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

servo/webrender#3393

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

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

servo/webrender#3393

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

UltraBlame original commit: 36ba5f47e949afe13875220f5cbbbca401d7be4c
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.

None yet

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