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

Reduce size of spatial node index #3438

Merged
merged 1 commit into from Jan 3, 2019

Conversation

@djg
Copy link
Contributor

djg commented Dec 19, 2018

Fixes #3431.


This change is Reviewable

@djg
Copy link
Contributor Author

djg commented Dec 19, 2018

@gw3583 would you care to review this one too?

@djg djg force-pushed the djg:reduce_size_of_SpatialNodeIndex branch 2 times, most recently from 168cc14 to 4d0b3bf Dec 19, 2018
@zptan
Copy link

zptan commented Dec 21, 2018

@kvark @gw r?

@djg Is it better to have a try run?

}
}

unsafe impl Send for SpatialNodeIndex {}

This comment has been minimized.

@emilio

emilio Dec 21, 2018

Member

How's this needed? It should be Send by default anyway, right?

Copy link
Member

kvark left a comment

This really does much more than just "Reduce size of spatial node index", introducing the IndexVec abstraction, which I have mixed feelings about. I wish we could separate these two and land/discuss independenty.

Reviewed 15 of 15 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @emilio and @djg)


webrender/src/clip_scroll_tree.rs, line 52 at r1 (raw file):

impl Idx for SpatialNodeIndex {
    fn new(value: usize) -> Self {
        assert!(value < u32::max_value() as usize);

nit: debug_assert!?


webrender/src/clip_scroll_tree.rs, line 61 at r1 (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

How's this needed? It should be Send by default anyway, right?

agreed, this shouldn't need any unsafe impls


webrender/src/gpu_types.rs, line 473 at r1 (raw file):

            from_index
        } else if from_index == to_index {
            SpatialNodeIndex::new(0)

perhaps, use ROOT_SPATIAL_NODE_INDEX here instead?


webrender/src/gpu_types.rs, line 648 at r1 (raw file):

        let index = metadatas.push(metadata);
        let _index2 = transforms.push(data);
        assert!(index == _index2);

nit: assert_eq!. Also, we shouldn't mark it as unused with _ since it's used in the assertion


webrender/src/index_vec.rs, line 31 at r1 (raw file):

#[cfg_attr(feature = "capture", derive(Serialize))]
#[cfg_attr(feature = "replay", derive(Deserialize))]
pub struct IndexVec<I: Idx, T> {

I'm not convinced this is a helpful abstraction. It carries almost zero logic in itself. WR has used u32 indices (newtyped for safety) for a lot of stuff, and that hasn't been a problem (i.e. not a burden to write, and not a source of mistakes).

@bors-servo
Copy link
Contributor

bors-servo commented Dec 22, 2018

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

@djg djg force-pushed the djg:reduce_size_of_SpatialNodeIndex branch from 4d0b3bf to 2f79d63 Jan 2, 2019
@djg
Copy link
Contributor Author

djg commented Jan 2, 2019

I've re-written the patch to not use type safe index nor the Vec wrapper.

@gw3583
Copy link
Collaborator

gw3583 commented Jan 3, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2019

📌 Commit 2f79d63 has been approved by gw3583

@bors-servo
Copy link
Contributor

bors-servo commented Jan 3, 2019

Testing commit 2f79d63 with merge 075fd68...

bors-servo added a commit that referenced this pull request Jan 3, 2019
Reduce size of spatial node index

Fixes #3431.

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

bors-servo commented Jan 3, 2019

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

@bors-servo bors-servo merged commit 2f79d63 into servo:master Jan 3, 2019
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 8 files, 5 discussions left (djg, emilio, 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 Jan 3, 2019
…caa408adcfcb (WR PR #3438). r=kats

servo/webrender#3438

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

--HG--
extra : moz-landing-system : lando
mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Jan 3, 2019
@djg djg deleted the djg:reduce_size_of_SpatialNodeIndex branch Jan 10, 2019
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…caa408adcfcb (WR PR #3438). r=kats

servo/webrender#3438

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

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

servo/webrender#3438

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

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

servo/webrender#3438

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

UltraBlame original commit: 094763412dbf5de18f04c0e37af9a9679b16c73b
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

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