Fix T93008: Cycles: Huge memory spike when saving tile to disk
ClosedPublic
Actions

Authored by Sergey Sharybin (sergey) on Nov 11 2021, 12:03 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
William Leeson (leesonw)

Commits

rC885e2eff499e: Fix T93008: Cycles: Huge memory spike when saving tile to disk
rBde8a46c6ad9c: Fix T93008: Cycles: Huge memory spike when saving tile to disk

Summary

The root of the problem lies in bug in OIIO which we can work around
from our side (which does not affect pack memory usage).

Thanks Brecht for finding the root cause!

NOTE: Since it is bcon3 I'd like to have independent testing :)

Diff Detail

Repository: rB Blender

Event Timeline

Sergey Sharybin (sergey) requested review of this revision.Nov 11 2021, 12:03 PM

Sergey Sharybin (sergey) created this revision.

Harbormaster completed remote builds in B18589: Diff 44616.Nov 11 2021, 12:03 PM

Alaska (Alaska) awarded a token.Nov 11 2021, 12:15 PM

silex (silex) awarded a token.Nov 11 2021, 12:38 PM

Looks good to me. The perfectionist in me would like to refactor out the for loops so that they are not repeated in the 2 methods. That way you only need to change one thing if there is a issue later on.

This revision is now accepted and ready to land.Nov 11 2021, 4:33 PM

This makes saving EXR files measurably slower: P2589.

From investigating the cause of memory usage, it appears to be ImageOutput::to_native_rectangle in OpenImageIO doing this. The reason it's doing a conversion is because of the overscan around tiles.

We can likely avoid the problem by writing the big tile in one go, but copied to a new buffer without the overscan. To save memory, this could potentially also be done in-place in the existing buffer, though I'm not sure if that's safe.

The reason it allocates so much memory is a bug in OpenImageIO:
https://github.com/OpenImageIO/oiio/pull/3176

Solution which does not syuffer from performance impact.

Modifying the render buffer in-place I'd avoid for 3.0 at least. Is not impossible to do so, but it does require modifications in the PathTrace::render_pipeline and it also could be considered an optimization than a bug fix.

Harbormaster completed remote builds in B18605: Diff 44648.Nov 11 2021, 8:22 PM

Sergey Sharybin (sergey) edited the summary of this revision. (Show Details)Nov 11 2021, 8:22 PM

Sergey Sharybin (sergey) edited the summary of this revision. (Show Details)

Seems to work for me.

kursad k (kursadk) awarded a token.Nov 11 2021, 9:20 PM

Seems fine. I ran a few tests and the perf looks fine as does the memory usage.

Closed by commit rBde8a46c6ad9c: Fix T93008: Cycles: Huge memory spike when saving tile to disk (authored by Sergey Sharybin (sergey)). · Explain WhyNov 12 2021, 11:52 AM

This revision was automatically updated to reflect the committed changes.

Sergey Sharybin (sergey) added a commit: rBde8a46c6ad9c: Fix T93008: Cycles: Huge memory spike when saving tile to disk.

Brecht Van Lommel (brecht) added a commit: rC885e2eff499e: Fix T93008: Cycles: Huge memory spike when saving tile to disk.Jan 27 2022, 11:31 PM