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
Branch
tile_sorting

Event Timeline

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

Nice time saving.

intern/cycles/util/util_list.h
21

We already have util_map.h

Sergey Sharybin (sergey) requested changes to this revision.Dec 22 2015, 2:58 PM
Sergey Sharybin (sergey) edited edge metadata.

First round of quick review.

intern/cycles/render/tile.cpp
42

Use prefix increment.

84

Public API is to have a proper name, private/protected stuff is to have underscore. See how it's done in Libmv i.e.

85

One member initialization at a line.

108

If it's a class, make those protected.

121

Speaking of duplication, this is duplication.

133

Why not to do:

state.tiles.clear();
state.tiles.resize(silced? num_devices: 1);
for(...) {
  list<Tile>& tile_list = state.tiles[i];
}

?

181

get_tiles() can return number of tiles.

intern/cycles/render/tile.h
67

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
121

True, I'll add a clear_tiles function.

intern/cycles/render/tile.h
67

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

True, std::map is not even needed anymore in the patch.

Lukas Stockner (lukasstockner97) edited edge metadata.
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
68

Fullstop.

85

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.
112–113

Fullstop.

Lukas Stockner (lukasstockner97) edited edge metadata.

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

Sergey Sharybin (sergey) edited edge metadata.

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) edited edge metadata.

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.