WIP upload instance data in RenderBackend

[zakorgy]

This addresses ##219
It works but it's still in a WIP state, because there are lot of duplication and the GL code path is broken now, I have to fix that.

---

This change is 

[kvark]

Great progress here, I'm excited to see this happening!
I did a surface review here, not a comprehensive one in any way. Will definitely come back, perhaps after a round of fixes in the PR.

Ideal case

One thing that I would love to see in the end is: get the instance data as early as possible, even before PrimitiveInstanceData is constructed, which you can see as PrimitiveInstanceData::from(instance), in the code in a few places.
Instead, this place would do something like this:

let mut instance_submanager = instance_manager.select(batch_key); // sets up the context in which only instances for this type of a batch are going to be pushed.
for instance in some_instance_list {
  instance_submanager.push(instance);
}

Where all the heavy lifting is done inside the push:

• checking if there is an open range of instances being recorded, and if not - starting one
• conversion Into<PrimitiveInstancedata> when it's written to mapped memory

Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @zakorgy)

---

webrender/src/batch.rs, line 344 at r1 (raw file):

pub struct PrimitiveBatch {
    pub key: BatchKey,
    pub instances: Vec<PrimitiveInstanceData>,

At the end of the day, we wouldn't need this instances vector at all: we'd be just writing them directly into the mapped buffers.

---

webrender/src/batch.rs, line 388 at r1 (raw file):

        heaps: Arc<Mutex<Heaps<B>>>,
    ) {
        let instances = self.instances.drain(..).map(|i| i.to_primitive_type()).collect::<Vec<_>>();

we should avoid collect here by all costs

---

webrender/src/batch.rs, line 392 at r1 (raw file):

            device,
            &instances,
            &mut heaps.lock().unwrap(),

should we move the lock() higher up in the stack? this function could just work with &mut Heaps<B>

---

webrender/src/batch.rs, line 2828 at r1 (raw file):

    /// Rectangle draws fill up the rectangles with rounded corners.
    pub slow_rectangles: Vec<ClipMaskInstance>,
    pub slow_rectangle_locations: Vec<InstanceLocation>,

it sounds like we need a structure combining both instances and locations, there is too many repetition here

---

webrender/src/batch.rs, line 2938 at r1 (raw file):

        buffer_manager: &mut InstanceBufferManager<B>,
        device: &B::Device,
        heaps: Arc<Mutex<Heaps<B>>>,

and if you do need to pass a mutex unlocked, pass it by reference instead of Arc, i.e. this place could be &Mutex<...>

---

webrender/src/frame_builder.rs, line 880 at r1 (raw file):

                            debug_assert!(batch_containers.is_empty());

                            alpha_batch_container.build(

one one hand this "build" has entirely different set of parameters from the exisint one

---

webrender/src/frame_builder.rs, line 919 at r1 (raw file):

                z_generator,
                composite_state,
                buffer_manager,

would it be feasible to add these three members to RenderTargetContext which is already passed here in the first argument?

---

webrender/src/internal_types.rs, line 542 at r1 (raw file):

        TextureUpdateList,
        BackendProfileCounters,
        FastHashMap<BufferId, InstancePoolBuffer<B>>,

can this be a part of RenderedDocument?

---

webrender/src/device/gfx/buffer.rs, line 533 at r1 (raw file):

    pub(super) fn still_in_flight(&self, frame_id: GpuFrameId, frame_count: usize) -> bool {
        for i in 0..frame_count {
            if self.bound_in_frame == GpuFrameId(frame_id.0 - i) {

can this be a simple comparison, aka self.bound_in_frame.0 + frame_count <= frame_id.0?

---

webrender/src/device/gfx/buffer.rs, line 860 at r1 (raw file):

                self.current_buffer_mut().align_offset_to(data_stride);
            }
            let update_size =

let's rename to upload_count, since size implies the byte size

---

webrender/src/device/gfx/buffer.rs, line 872 at r1 (raw file):

            let start = end - self.current_buffer().last_update_size / data_stride;
            locations.push(InstanceLocation {
                buffer_id: BufferId(self.next_id.0 - 1),

nit: the logic of tracking what the current index/buffer is should be isolated, i.e. this should beself.current_buffer_id() method

---

webrender/src/device/gfx/device.rs, line 1588 at r1 (raw file):

                let mut buffer_ids = locations.iter().map(|l| l.buffer_id).collect::<Vec<_>>();
                buffer_ids.sort();
                buffer_ids.dedup();

why do we need to sort and deduplicate it? Perhaps, we could just iterate the original instead?

---

webrender/src/device/gfx/device.rs, line 3199 at r1 (raw file):

    ) {
        debug_assert!(self.inside_frame);
        self.draw(None);

we aren't using the new parameter yet?

---

webrender/src/device/gfx/program.rs, line 54 at r1 (raw file):

}

impl<'a, B: hal::Backend> From<(&'a InstanceBufferHandler<B>, std::ops::Range<usize>)> for InstanceSource<'a, B> {

let's make it a method/constructor of InstanceSource instead of playing with fire traits

---

webrender/src/device/gfx/program.rs, line 748 at r1 (raw file):

                                cmd_buffer.bind_vertex_buffers(
                                    0,
                                    Some((&vertex_buffer.buffer().buffer, 0))

nit: iter::once() would be better here

---

webrender/src/device/gfx/program.rs, line 748 at r1 (raw file):

                                cmd_buffer.bind_vertex_buffers(
                                    0,
                                    Some((&vertex_buffer.buffer().buffer, 0))

also, we shouldn't be rebinging the vertex buffer, we only need to bind the instance one

[zakorgy]

@kvark I've started addressing your comments but I'm a little far from what you described at the top of the review. After I've finished addressing those comments I will answer them as well.