Cycles: Sort tiles in rendering order at construction time
ClosedPublic
Actions

Authored by Lukas Stockner (lukasstockner97) on Dec 22 2015, 2:14 PM.

Details

Reviewers

Thomas Dinges (dingto)
Sergey Sharybin (sergey)

Commits

rBS548eb9eb4bdd: Cycles: Sort tiles in rendering order at construction time
rCf123db1aadba: Sort tiles in rendering order at construction time
rB548eb9eb4bdd: Cycles: Sort tiles in rendering order at construction time

Summary

This commit modifies the TileManager to sort render tiles once after tiling the image,
instead of searching the next tile every time a new tile is acquired by a device.

This makes acquiring a tile run in constant time, therefore the render time is linear
w.r.t. the amount of tiles, instead of the quadratic dependency before.

Furthermore, each (logical) device now has its own Tile list, which makes acquiring
a tile for a specific device easier.
Also, some code in the TileManager was deduplicated.

For the Classroom benchmark at 1080p, 1spp and 8x8 tiles, the time reduction was about 20% iirc.

Diff Detail

Repository: rB Blender

Event Timeline

Lukas Stockner (lukasstockner97) updated this revision to Diff 5611.Dec 22 2015, 2:14 PM

Lukas Stockner (lukasstockner97) retitled this revision from to Cycles: Sort tiles in rendering order at construction time.

Lukas Stockner (lukasstockner97) added reviewers: Thomas Dinges (dingto), Sergey Sharybin (sergey).

Lukas Stockner (lukasstockner97) updated this object.

Nice time saving.

intern/cycles/util/util_list.h
21 ↗	(On Diff #5611)	We already have util_map.h

First round of quick review.

intern/cycles/render/tile.cpp
81	Use prefix increment.
133–134	If it's a class, make those protected.
134	Speaking of duplication, this is duplication.
135	Public API is to have a proper name, private/protected stuff is to have underscore. See how it's done in Libmv i.e.
135	Why not to do: state.tiles.clear(); state.tiles.resize(silced? num_devices: 1); for(...) { list<Tile>& tile_list = state.tiles[i]; } ?
136	One member initialization at a line.
177	`get_tiles()` can return number of tiles.
intern/cycles/render/tile.h
66–68	Please do comment such non-intuitive mappings. Also not really sure why it should be a pointer to the list?

This revision now requires changes to proceed.Dec 22 2015, 2:58 PM

Answered, I'll update the patch.

intern/cycles/render/tile.cpp
134	True, I'll add a clear_tiles function.
intern/cycles/render/tile.h
66–68	I went for pointers to avoid copying the list when pushing it into the vector, but with the resize approach you mentioned below, that's not necessary anymore
intern/cycles/util/util_list.h
21 ↗	(On Diff #5611)	True, std::map is not even needed anymore in the patch.

Review should be adressed with this version.

Aaron Carlisle (Blendify) awarded a token.Dec 22 2015, 3:48 PM

Seems fine apart from the TileComparator changes. Perhaps you misunderstood them? More detailed notes are inlined.

intern/cycles/render/tile.h
67	Fullstop.
86	Perhaps you misunderstood the note. What i meant is: TileComparator is to have protected `order_` and `center_` fields TileComparator constructor would have `(Tile &a, Tile &b)` signature Whole `TileComparator` definition could be moved to an anonymous namespace at the top of `tile.cpp`.
113–114	Fullstop.