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

Port linear gradients to be interned primitives. #3352

Merged
merged 2 commits into from Nov 27, 2018

Conversation

@gw3583
Copy link
Collaborator

gw3583 commented Nov 26, 2018

This change is Reviewable

@gw3583
Copy link
Collaborator Author

gw3583 commented Nov 26, 2018

This patch is not tidy, but not quite as bad as it looks, since it includes #3349 and #3350 - probably best to review this after those have been reviewed and merged.

Pending try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdad847f558535e816874e76b788bed727a7707e

@gw3583
Copy link
Collaborator Author

gw3583 commented Nov 26, 2018

Try run looks good so far. Also seems to be about another 2% win in dl_mutate (https://treeherder.mozilla.org/perf.html#/graphs?series=try,1660472,1,1&selected=try,1660472,407537,652574374).

@kvark
kvark approved these changes Nov 26, 2018
Copy link
Member

kvark left a comment

:lgtm: a few suggestions are below

Reviewed 8 of 8 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gw3583)


webrender/src/border.rs, line 1133 at r3 (raw file):

pub fn create_nine_patch_segments(
    rect: &LayoutRect,
    nine_patch: &NinePatchDescriptor,

could this be a method of this descriptor?


webrender/src/prim_store.rs, line 422 at r3 (raw file):

impl Eq for GradientStopKey {}

impl hash::Hash for GradientStopKey {

would it be easier to just introduce a HashFloat? we'd not need to manually derive Hash for anything else then. We'd even use TypedSize2D<HashFloat, ...> and TypedPoint2D<HashFloat, ..> for a number of things here


webrender/src/prim_store.rs, line 2691 at r3 (raw file):

pub type ImageInstanceStorage = storage::Storage<ImageInstance>;
pub type ImageInstanceIndex = storage::Index<ImageInstance>;
pub type GradientTileStorage = storage::Storage<VisibleGradientTile>;

why do we need all those typedefs? IIRC, they were helpful when the indices weren't strongly typed. Now everything is explicit enough. Is it just to save on symbol count?

@bors-servo
Copy link
Contributor

bors-servo commented Nov 26, 2018

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

@gw3583 gw3583 force-pushed the gw3583:intern-gradient-3 branch from c93962a to eb8381b Nov 26, 2018
Copy link
Collaborator Author

gw3583 left a comment

Reviewable status: 0 of 8 files reviewed, 3 unresolved discussions (waiting on @kvark)


webrender/src/border.rs, line 1133 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

could this be a method of this descriptor?

Yep, done.


webrender/src/prim_store.rs, line 422 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would it be easier to just introduce a HashFloat? we'd not need to manually derive Hash for anything else then. We'd even use TypedSize2D<HashFloat, ...> and TypedPoint2D<HashFloat, ..> for a number of things here

Yep, sounds good. That's probably a good approach to switch to when tidying all this up.


webrender/src/prim_store.rs, line 2691 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why do we need all those typedefs? IIRC, they were helpful when the indices weren't strongly typed. Now everything is explicit enough. Is it just to save on symbol count?

Initially I was just following the initial code which had typedefs, but I agree - I'm fine with changing them to be explicit types. OK if we do that later on, to avoid rebase issues with the other open PRs?

@gw3583 gw3583 force-pushed the gw3583:intern-gradient-3 branch from eb8381b to b9dda49 Nov 26, 2018
@gw3583
Copy link
Collaborator Author

gw3583 commented Nov 26, 2018

Rebased and fixed one of the review comments. I agree with the other two suggestions too - although would prefer to do those as follow ups, to avoid rebase issues with other open patches.

@kvark
Copy link
Member

kvark commented Nov 27, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Nov 27, 2018

📌 Commit b9dda49 has been approved by kvark

bors-servo added a commit that referenced this pull request Nov 27, 2018
Port linear gradients to be interned primitives.

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

bors-servo commented Nov 27, 2018

Testing commit b9dda49 with merge 604c69a...

@kvark
Copy link
Member

kvark commented Nov 27, 2018

My things TODO after picture caching is here is growing and growing ;)

@gw3583
Copy link
Collaborator Author

gw3583 commented Nov 27, 2018

My things TODO after picture caching is here is growing and growing ;)

#3357

@bors-servo
Copy link
Contributor

bors-servo commented Nov 27, 2018

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

@bors-servo bors-servo merged commit b9dda49 into servo:master Nov 27, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 4 files, 3 discussions left (kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 27, 2018
…a6e5edfe73f4 (WR PR #3352). r=kats

servo/webrender#3352

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

--HG--
extra : moz-landing-system : lando
jankeromnes pushed a commit to jankeromnes/gecko that referenced this pull request Nov 27, 2018
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…a6e5edfe73f4 (WR PR #3352). r=kats

servo/webrender#3352

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

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

servo/webrender#3352

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

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

servo/webrender#3352

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

UltraBlame original commit: 2f8233496ee8d83688e27d266e5dd61ba29ad5b1
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.