Reduce lock stepping caused by profiling on CPU.
AbandonedPublic
Actions

Authored by William Leeson (leesonw) on Nov 11 2021, 11:32 AM.

Details

Reviewers

Sergey Sharybin (sergey)
Brecht Van Lommel (brecht)

Maniphest Tasks

T92601: Super LOW performance on CPU rendering in complex scene

Summary

Only use profiling when it is enabled otherwise don't use it.

Some performance data on the three patches

patch	Juans Scene UI 256 samples	Juans Scene bg 256 samples	junkshop UI	junkshop bg
No patch	6:16.59	4:05.37	2:08.48	1:59.7
D13187	4:12.15	3:57.36	2:07.25	1:58.16
D13185	4.11.18	3:54.74	2:07.44	1:58.03
D13190	4:12.39	3:55.42	2:07.62	1:58.68

UI - means rendered from within Blender
bg - means rendered from the command line using blender -b scene.blend -f 1

Diff Detail

Repository

rB Blender

Branch

slowCPU (branched from master)

Build Status

Buildable 18588
Build 18588: arc lint + arc unit

Event Timeline

William Leeson (leesonw) requested review of this revision.Nov 11 2021, 11:32 AM

William Leeson (leesonw) created this revision.

Harbormaster completed remote builds in B18588: Diff 44615.Nov 11 2021, 11:33 AM

William Leeson (leesonw) added a task: T92601: Super LOW performance on CPU rendering in complex scene.Nov 11 2021, 11:34 AM

William Leeson (leesonw) added reviewers: Sergey Sharybin (sergey), Brecht Van Lommel (brecht).

William Leeson (leesonw) added a project: Render & Cycles.

William Leeson (leesonw) added a subscriber: Juan Gea (juang3d).

For the performance related changes I'd expect to see some quantification and how it was obtained.
My guess that this is part of T92601. Does this change bring any performance improvement?

intern/cycles/integrator/path_trace.cpp
402	Avoid white-space changes.
781	Same as above.
intern/cycles/integrator/path_trace.h
60	Here and in other places the indentation is broken: tabs are used instead of spaces. You might want to configure the IDE to use spaces, and also re-ensure clang-format is used.
intern/cycles/integrator/path_trace_work_cpu.cpp
83	Code style.
89	Avoid dead code.
116	Code style.
intern/cycles/integrator/shader_eval.cpp
109–113	This will make it impossible to quickly cancel evaluation, Might not be that problematic for F12 renders, but for viewport it could introduce unwanted lag in refresh rate. If this is a hotspot we should consider doing something similar to `PathTraceCPU` where the check is done based on boolean flag.

This revision now requires changes to proceed.Nov 11 2021, 11:49 AM

Fix formatting.

Harbormaster completed remote builds in B18590: Diff 44618.Nov 11 2021, 1:00 PM

For the case where profiling is enabled we should try to improve performance, but that can be done separate from this bugfix. Probably we can only mark the state as active/inactive PathTraceWorkCPU::render_samples, and do the intialization/merging once per render.

intern/cycles/integrator/path_trace.h
60	I don't think we have to pass in `use_profiling`. I think instead `Profiler::add_state` and `Profiler::remove_state` can check if the profiler has been started, and if not, do nothing.

Removed dead code and restored cancel.

Harbormaster completed remote builds in B18591: Diff 44619.Nov 11 2021, 1:08 PM

Alaska (Alaska) awarded a token.Nov 11 2021, 1:21 PM

Some performance data on the three patches

patch	Juans Scene UI 256 samples	Juans Scene bg 256 samples	junkshop UI	junkshop bg
No patch	6:16.59	4:05.37	2:08.48	1:59.7
D13187	4:12.15	3:57.36	2:07.25	1:58.16
D13185	4.11.18	3:54.74	2:07.44	1:58.03
D13190	4:12.39	3:55.42	2:07.62	1:58.68

UI - means rendered from within Blender
bg - means rendered from the command line using blender -b scene.blend -f 1

William Leeson (leesonw) mentioned this in D13187: add_state and remove_state only work when the profiler is running..Nov 11 2021, 2:25 PM

Did you accidentally swap some columns? Not sure why this patch would make UI rendering slower otherwise.

Thanks for the number! Suggest moving them to the patch description (which will also end up in the commit message, communicating results to those who follow those logs ;).

The background mode speedup seems impressive! But what's up with the UI case form the first column becoming slower?

If we can avoid passing use_profiling that'd be great.

We can avoid passing use_profiling and avoid the per-thread overhead by adding an Profiler::active() { return worker != nullptr; } method and then checking if (device_->profiler.active()).

Yes @Sergey Sharybin (sergey) I had swapped the columns not sure what happened there. @Brecht Van Lommel (brecht) I'll try the method to the device to see what happens.

William Leeson (leesonw) mentioned this in D13190: Fix T92601: Disable profiling when the profiler is deemed not active..Nov 11 2021, 3:55 PM

William Leeson (leesonw) edited the summary of this revision. (Show Details)

William Leeson (leesonw) mentioned this in rB32c768785991: Fix T92601: Disable profiling when the profiler is deemed not active..Nov 12 2021, 10:08 AM

William Leeson (leesonw) mentioned this in rB456876208b16: Fix T92601: Disable profiling when the profiler is deemed not active..Nov 12 2021, 10:22 AM

It was decided to go with D13190 instead.

Brecht Van Lommel (brecht) mentioned this in rC64337d890b30: Fix T92601: Disable profiling when the profiler is deemed not active..Jan 27 2022, 11:31 PM