api: use shared_ptr for ImageCache and TextureSystem

[lgritz]

Change IC::create() and TS::create() to return shared_ptr instead of a raw pointer. This cleans up a lot of lingering lifetime management issue of these classes. The switch to 3.0 is an opportunity to make breaking changes to these APIs.

For the sake of downstream users, we define preprocessor symbols
OIIO_IMAGECACHE_CREATE_SHARED and OIIO_TEXTURESYSTEM_CREATE_SHARED to signal that the new APIs are present. Some very minor changes may be needed at the call site.

[fpsunflower]

One thing to watch out for, is that passing std::shared_ptr by value will twiddle the reference count unnecessarily. In pathological cases it could lead to false sharing type performance bugs. It also signals the function might be trying to keep its own internal reference, which I don't think is the intent here.

I think passing the shared_ptr by const reference is the recommended idiom for cases like these where you don't intend to touch the shared pointer itself, just want to use the object it points to.

Some related articles:

https://herbsutter.com/2013/06/05/gotw-91-solution-smart-pointer-parameters/
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-uniqueptrparam

[fpsunflower]

On reading the reset of the review more carefully, it looks like ImageBuf, does need to keep a reference to the cache, so it looks like the by-value semantics are right here. Still, its worth keeping in mind the hidden cost of the reference count being twiddled at each function call (more than it would have been before).

[lgritz]

Specifically to this place in the code: it won't make a difference, this is in idiff, there is no concurrency here and only a few calls to this function per run.

But I take your point generally. In a case like this, do you think it's better to pass as const shared_ptr<T>& or just as a T*, knowing that the shared_ptr from which it came is guaranteed to outlive the function call?

[lgritz]

On reading the reset of the review more carefully, it looks like ImageBuf, does need to keep a reference to the cache, so it looks like the by-value semantics are right here. Still, its worth keeping in mind the hidden cost of the reference count being twiddled at each function call (more than it would have been before).

If we passed a const shared_ptr<T>& to ImageBuf::reset(), actually, that would be fine because it would still properly make a new shared_ptr internal to the IB, while avoiding the ref count diddling on the way in and out of the call. On the other hand, reset() (much like the constructor) is called, very infrequently if ever (after all, it's basically throwing out the contents of the IB and then reusing or reallocating it for something totally different). So I'm not sure there is a performance issue here in actuality.

I definitely agree that if there is any API call that we expect to be calling in a tight loop and it needs access to the cache for informational purposes (not to take ownership), in that case it's very important to pass either a reference to the shared_ptr, or else a regular pointer from ptr.get().

[lgritz]

I will look, too, but please help me double check for instances of API calls that

1. Might be called very frequently.
2. Never need to take ownership or make a second, lasting, copy of the shared_ptr.

We should ensure that in such instances, we are passing a reference or a non-owning raw pointer.

[fpsunflower]

I'm guessing the call to reset is to force the destruction to happen before shutdown as opposed to implicitly on end of scope? Maybe leave a comment about this, otherwise it looks like a line that could be removed.

[fpsunflower]

In fact - since this is for the shared ImageCache object, the real "last" owner of the image cache is the global variable, so if I am thinking about this correctly, this line doesn't really accomplish much?

[lgritz]

yes, and will do

[lgritz]

Should this be const std::shared_ptr<ImageCache>&? The ctr can still internally capture a new shared_ptr to take shared ownership of the cache, but by passing a ref, it avoids two extra ref count adjustments (in and out of the function call). For something called as infrequently as an IB ctr, it's not clear to me if avoiding two ref count adjustments Or is the answer

[lgritz]

N/M, I see now: pass by value + use std::move within the ctr will eliminate any extraneous ref count adjustment.

[lgritz]

OK, I think maybe I have a plan. Tell me what you think, @fpsunflower :

If you have a method that takes a shared_ptr and will definitely keep an internal copy, pass by value shared_ptr<T> then use std::move to store it internally so as to avoid any unnecessary ref count adjustment.

If you have a method that is merely an observer of the pointed-to shared object and never assumes ownership or needs to retain a reference/pointer to the object, pass a T* or const T* for max performance if there are many internal uses of the object (OR if you want the method to also work for a non-shared object), or a const shared_ptr<T>& reference if you don't mind the cost of the extra indirection and know it will ALWAYS be called by somebody who has a shared_ptr and not a raw pointer.

If you have a method that takes a shared_ptr and has logic whereby it MAY OR MAY NOT store a copy internally (depending on conditions)... what's the best route? If you pass the shared_ptr by value, you'll have unnecessary ref count diddling in the case where you don't keep an internal copy, but if you pass by reference, you have an extra level of pointer indirection on every use of it. Probably a wash, but do you have a "clarity" based preference?

[lgritz]

I think what I said corresponds to https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r36-take-a-const-shared_ptrwidget-parameter-to-express-that-it-might-retain-a-reference-count-to-the-object-

[lgritz]

Updated, in the end I kept the value semantics because IB ctr/reset will retain shared ownership of any IC they are passed, but I did add a couple std::move wrappers to the assignments to ensure no unnecessary ref count adjustments in the process.

[fpsunflower]

This looks good to me. It would be nice to have a OIIO focused performance testsuite, though I suspect in practice the performance risk is fairly minimal unless you are doing something like creating tons of ImageBufs from lots of threads.

[fpsunflower]

For the sake of discussion - if I was designing the API from scratch, I would probably opt for std::unique_ptr<ImageCache> and then all other uses would be raw pointers (which following the modern C++ idioms indicates a non-owning pointer).

I would also advocate for removing the bool shared arg from the create() calls, and just rely on the user of the library to manage their (single) cache object. This would avoid hidden state and improve control over when memory is released. With the global static std::shared_ptr<ImageCache> shared_image_cache; singleton, you only can truly destroy it when the library is closed (although calls to invalidate_all() can help release the bulk of it).

Anyways, I'm not necessarily pushing for this approach since we need to keep in mind backwards compatibility to some extent, but since I'm guessing this is aimed at 3.0 -- its maybe worth discussing the pros/cons?

[lgritz]

I suspect in practice the performance risk is fairly minimal unless you are doing something like creating tons of ImageBufs from lots of threads.

Yes, any way you shake it, per image operations (like create a new ImageBuf) are very infrequent compared to per-pixel operations on those images.

since I'm guessing this is aimed at 3.0 -- its maybe worth discussing the pros/cons?

Yes this is 3.0 as even this change requires an API break. Definitely worth discussing other possible changes.

I think unique_ptr is a bad fit, because part of what I want to do is solve the problem of figuring out the relative destruction order of the several things that might retain a handle to an IC -- various IBs, a TextureSystem, maybe regular user code. Making them all have shared pointers does the thing that ref counting is good at: destroys the IC when, and only when, nobody is left who expects to still reference it.

I like the idea of a shared cache, but I do agree with the fact that the 'shared' flag can go away if we add a call to return a handle to the singleton global cache. Then a caller can either ask for the shared one, or ask to create one (which will always get you a new one). I might do that as a later step.

[lgritz]

Speaking of which, I do have a follow-on after this is merged, which is that I'm going to propose that IC and TS be "de-virtualized" and instead use a PIMPL idiom like IB already does.

IC and TS were set up initially as pure virtual interface classes so that something like OSL could use its interface while allowing a renderer to substitute a different implementation. But in practice, I think there's a better mechanism for that -- OSL's RendererServices (or new/in progress "free functions" supplied by the renderer as LLVM bitcode) already wrap the texture calls, and so don't need to conform to the OIIO interfaces at all if they don't really want to. But the cost we're paying for this flexibility we don't need is that we can't amend the public IC or TS interfaces at all between major releases, since even the addition of a new virtual function can change the vtbl and break ABI.

So what I want to do is add an opaque PIMPL ptr, and make as much of the public interface be ordinary methods, not virtual, which will allow us to freely add to the interface without ABI breaks until we need remove a call entirely, which of course we can always wait to do until a scheduled ABI-breaking release.

Is anybody RELYING on the current design of IC and TS being a pure virtual facade interface class, backed by a hidden implementation subclass that overloads every single call?

[lgritz]

@sfriedmapixar Does this raise any reg flags for you? Especially my last comment above about what I'd like to do next? Do you rely on the current abstract interface class design, or do you substitute in a different texturesystem via the RendererServices layer?

[lgritz]

We discussed this at today's OSL TSC meeting, and @sfriedmapixar says he's not going to be inconvenienced by this PR or the follow-up I mentioned about de-virtualizing these classes.

Anybody else have concerns?