Costly allocation and shared_ptr overuse in ray_color function

[pdani86]

I managed to reduce render time of "theRestOfYourLife" example from ~42 to ~35 secs with the following modifications:
(the code below is only for a quick proof of concept, it's not the way I would leave it)

1. avoided allocation in ray_color function:

// changed this:
auto light_ptr = make_shared<hittable_pdf>(lights, rec.p);
mixture_pdf p(light_ptr, srec.pdf_ptr);
// to this:
auto light_ptr = hittable_pdf(lights, rec.p); // not really a "_ptr"
mixture_pdf p(light_ptr, *srec.pdf_ptr);

Also had to change underlying classes to store ptr-s, or references (in hittable_pdf and mixture_pdf) instead of shared_ptr-s. (This might not be the best solution, because it needs the user of the mixture_pdf class to know that the lifetime of the passed in objects is not prolonged by mixture_pdf object.)

2. reduced parameter count of ray_color function by putting it in a class:

class RC {
public:
    RC(color background, hittable& world, hittable& lights)
        : background(background), world(world), lights(lights) {}

    color ray_color(
        const ray& r,
        int depth
    ) {
    // ...
    }

    color background;
    hittable& world;
    hittable& lights;
};

(world and lights could have stayed shared_ptr-s)

The performance gain with this comes from avoiding the costly (recursive) shared_ptr pass-by-value issue which could have been avoided also by passing it as shared_ptr&.

If I measured correctly, changing the original shared_ptr pass-by-value to shared_ptr pass-by-ref in ray_color function resulted in roughly 1% less render time.

[trevordblack]

I'll look into it.
But on the performance-simplicity axis, this looks like quite a bit more complex for very little performance gains.

Realistically we should touch on how to get your ray tracing to go quick. But it's never been a priority.

[pdani86]

Ok, I understand. (I got here by searching for a raytracing library, not for learning how to do it. :) )
I still would modify one thing, only one character change, and better practice:

Original on master:

color ray_color(
    const ray& r,
    const color& background,
    const hittable& world,
    shared_ptr<hittable> lights,
    int depth
) {

Pass lights parameter by reference:

color ray_color(
    const ray& r,
    const color& background,
    const hittable& world,
    const shared_ptr<hittable>& lights,
    int depth
) {

[hollasch]

See https://stackoverflow.com/questions/8385457/should-i-pass-a-shared-ptr-by-reference. Passing by reference is faster, but because it bypasses the functionality that a shared pointer was supposed to serve in the first place.

The original code allocated most objects and then never freed them. That works fine, as long as you're rendering a single image and then going home. It doesn't work as well if you decide to extend to multiple frames (rendering an animation, for example), or setting up some kind of service.

Rather than impose some kind of lifetime management, we decided to use shared_ptr<> everywhere to kick the issue off the table. As you note, it's often more management than you need, but such questions should be carefully considered in your specific implementation first.

The larger issue here is the intent of the source code. This is absolutely positively not production code. It's certainly not the way I would implement my ray tracer, even as a hobby. The code as it stands is primarily expository, meant to be clear and unambiguous to all readers, including those who are barely familiar with C++ and like it that way.

Another issue is "leaving enough meat on the bone" for the reader. Part of the joy of writing your own raytracer is adding your own features as you wish. It could really use some command-line switches, for example. It can definitely be sped up, and significantly. All of that said, simplicity will win every time for book source.

I'm glad you're changing things up in the pursuit of faster renders -- have fun and let us know how it goes!

[pdani86]

Thanks for your time. I really like the simplicity of the code.

[hollasch]

By the way, since the primary benefit of std::shared_ptr<> is lifetime management via automated C++ scoping (construction/destruction), and since you're throwing that management away by passing a reference, you might make your code much simpler by passing the bare pointer to children (via std::shared_ptr<>::get()). You'll still get managed deletion at the parent level, but children can just work with the bare pointer. Other options include factories or frame heaps, for even faster performance when allocating and deallocating tons of objects that can all go away when the frame is done.

[hollasch]

By the way, when I say "pointer" above, I meant in the loose sense. As you noted earlier, it would probably be a reference.

Finally, if you're interested in the thinking that led us to settle on std::shared_ptr<>, check out the discussion in #147.

The bottom line is that there are many ways to implement this, and readers are strongly encouraged to implement it as they please and play with the result. We debated for quite a while about having a buildable program, because of the strong unintended hint to the reader that the sample code was correct or good or recommended. What we're lacking right now is a clear, up-front section that makes it very clear that the codebase is an example toy implementation.

I've added #833 to give this better service.