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

Refactor and small optimization of batching code. #1600

Merged
merged 1 commit into from Aug 25, 2017

Conversation

@glennw
Copy link
Member

glennw commented Aug 23, 2017

This is prep work for some upcoming changes to support
different vertex formats per batch type. This will allow:

  1. Much smaller vertex format for glyph instances.
  2. Different vertex attribute types per batch type (e.g. using 2x u16 for GPU cache addresses).

There's also a small optimization - we no longer store the bounding
rects for items in the opaque batch lists, since we don't need
to check for overlap there.


This change is Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Aug 24, 2017

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

@glennw glennw force-pushed the glennw:batching branch from 32467d9 to f744ca1 Aug 24, 2017
@glennw
Copy link
Member Author

glennw commented Aug 24, 2017

Rebased

}
}

pub struct OpaqueBatchList {

This comment has been minimized.

@kvark

kvark Aug 24, 2017

Member

hmm, there appears to be quite a bit of duplicated code, on both the API and the implementation side.
Might be worth uniting those structs:

trait PrimitiveBatch: Sized {
  type Bound;
  fn new(&AlphaBatchKey) -> Self;
  fn check_intersections(&self, &Self::Bound) -> bool;
  fn add_bound(&mut self, &Self::Bound) -> &mut Vec<PrimitiveInstance>;
}
struct BatchList<B> {
  pub batches: Vec<B>,
}
impl<B: PrimitiveBatch> for BatchList<B> {
...
}

This comment has been minimized.

@glennw

glennw Aug 24, 2017

Author Member

Although the code is mostly duplicated right now, I'm planning to experiment with changing the batching strategies for opaque and alpha - I think they'll end up being quite different.

In particular, while the alpha batcher would stay quite similar, the opaque batcher would ensure very large primitives get rendered first (even if a batch break is involved), while smaller primitives could get added to a per-batch-type list, without having to search for an appropriate batch.

So I think it makes sense to leave the code duplicated for now - does that seem reasonable?

@kvark
Copy link
Member

kvark commented Aug 24, 2017

Sorry about the late review!
The PR does simplify a lot of things nicely, the least part I like is the duplication of get_suitable_batch code

@kvark
Copy link
Member

kvark commented Aug 25, 2017

Ok, looking forward to see the next steps ;) 👍
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Aug 25, 2017

📌 Commit f744ca1 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Aug 25, 2017

🔒 Merge conflict

@bors-servo
Copy link
Contributor

bors-servo commented Aug 25, 2017

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

This is prep work for some upcoming changes to support
different vertex formats per batch type. This will allow:

1) Much smaller vertex format for glyph instances.
2) Different vertex attribute types per batch type (e.g. using 2x u16 for GPU cache addresses).

There's also a small optimization - we no longer store the bounding
rects for items in the opaque batch lists, since we don't need
to check for overlap there.
@glennw glennw force-pushed the glennw:batching branch from f744ca1 to edf1cd2 Aug 25, 2017
@glennw
Copy link
Member Author

glennw commented Aug 25, 2017

Rebased.

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

bors-servo commented Aug 25, 2017

📌 Commit edf1cd2 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Aug 25, 2017

Testing commit edf1cd2 with merge dd11229...

bors-servo added a commit that referenced this pull request Aug 25, 2017
Refactor and small optimization of batching code.

This is prep work for some upcoming changes to support
different vertex formats per batch type. This will allow:

1) Much smaller vertex format for glyph instances.
2) Different vertex attribute types per batch type (e.g. using 2x u16 for GPU cache addresses).

There's also a small optimization - we no longer store the bounding
rects for items in the opaque batch lists, since we don't need
to check for overlap there.

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

bors-servo commented Aug 25, 2017

☀️ Test successful - status-travis
Approved by: kvark
Pushing dd11229 to master...

@bors-servo bors-servo merged commit edf1cd2 into servo:master Aug 25, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
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

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