Page MenuHome

Cycles: Sort tiles in rendering order at construction time
ClosedPublic

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

Details

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) retitled this revision from to Cycles: Sort tiles in rendering order at construction time.Dec 22 2015, 2:14 PM

Nice time saving.

intern/cycles/util/util_list.h
21 ↗(On Diff #5611)

We already have util_map.h

Sergey Sharybin (sergey) requested changes to this revision.

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.

Lukas Stockner (lukasstockner97) marked 3 inline comments as done.

Review should be adressed with this version.

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.

TileComparator is updated, thanks to @Sergey Sharybin (sergey) for the patch!

Sergey Sharybin (sergey) accepted this revision.

LGTM.

One thing tho -- i don't have time to apply the patch to comiler/test atm, so plase, do latest compile+run test before pushing ;)

This revision is now accepted and ready to land.Dec 23 2015, 12:48 PM
Thomas Dinges (dingto) accepted this revision.

LGTM. Tested quickly on OS X, simple scene with 8x8 tiles (1280x720), with 1 sample was faster. Went down from 9.6s to 8.1s. :)

This revision was automatically updated to reflect the committed changes.