Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: use shared_ptr for ImageCache and TextureSystem #4377

Merged
merged 3 commits into from
Aug 16, 2024
Merged

api: use shared_ptr for ImageCache and TextureSystem #4377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e37deb9
api: use shared_ptr for ImageCache and TextureSystem
lgritz Aug 12, 2024
61810df
Use std::move to avoid extra ref count incr/decr
lgritz Aug 14, 2024
7ac7644
Remove unnecessary IC reset
lgritz Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/iconvert/iconvert.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,8 +356,8 @@ convert_file(const std::string& in_filename, const std::string& out_filename)
// subimage appending, we gather them all first.
std::vector<ImageSpec> subimagespecs;
if (out->supports("multiimage") && !out->supports("appendsubimage")) {
ImageCache* imagecache = ImageCache::create();
int nsubimages = 0;
auto imagecache = ImageCache::create();
int nsubimages = 0;
ustring ufilename(in_filename);
imagecache->get_image_info(ufilename, 0, 0, ustring("subimages"),
TypeInt, &nsubimages);
Expand All @@ -370,7 +370,6 @@ convert_file(const std::string& in_filename, const std::string& out_filename)
adjust_spec(in.get(), out.get(), inspec, subimagespecs[i]);
}
}
ImageCache::destroy(imagecache);
}

bool ok = true;
Expand Down
8 changes: 4 additions & 4 deletions src/idiff/idiff.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,9 @@ getargs(int argc, char* argv[])


static bool
read_input(const std::string& filename, ImageBuf& img, ImageCache* cache,
int subimage = 0, int miplevel = 0)
read_input(const std::string& filename, ImageBuf& img,
std::shared_ptr<ImageCache> cache, int subimage = 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

int miplevel = 0)
{
if (img.subimage() >= 0 && img.subimage() == subimage
&& img.miplevel() == miplevel)
Expand Down Expand Up @@ -232,7 +233,7 @@ main(int argc, char* argv[])

// Create a private ImageCache so we can customize its cache size
// and instruct it store everything internally as floats.
ImageCache* imagecache = ImageCache::create(true);
std::shared_ptr<ImageCache> imagecache = ImageCache::create(true);
imagecache->attribute("forcefloat", 1);
if (sizeof(void*) == 4) // 32 bit or 64?
imagecache->attribute("max_memory_MB", 512.0);
Expand Down Expand Up @@ -418,7 +419,6 @@ main(int argc, char* argv[])
}

imagecache->invalidate_all(true);
ImageCache::destroy(imagecache);
shutdown();
return ret;
}
24 changes: 12 additions & 12 deletions src/include/OpenImageIO/imagebuf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ enum class InitializePixels { No = 0, Yes = 1 };
/// translate into a different data format than appears in the file).
///
/// ImageBuf data coming from disk files may optionally be backed by
/// ImageCache, by explicitly passing an `ImageCache*` to the ImageBuf
/// ImageCache, by explicitly passing an ImageCache to the ImageBuf
/// constructor or `reset()` method (pass `ImageCache::create()` to get a
/// pointer to the default global ImageCache), or by having previously set the
/// global OIIO attribute `"imagebuf:use_imagecache"` to a nonzero value. When
Expand Down Expand Up @@ -186,9 +186,9 @@ class OIIO_API ImageBuf {
/// ensure that it remains valid for the lifetime of the ImageBuf.
///
explicit ImageBuf(string_view name, int subimage = 0, int miplevel = 0,
ImageCache* imagecache = nullptr,
const ImageSpec* config = nullptr,
Filesystem::IOProxy* ioproxy = nullptr);
std::shared_ptr<ImageCache> imagecache = {},
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const ImageSpec* config = nullptr,
Filesystem::IOProxy* ioproxy = nullptr);

/// Construct a writable ImageBuf with the given specification
/// (including resolution, data type, metadata, etc.). The ImageBuf will
Expand Down Expand Up @@ -270,9 +270,9 @@ class OIIO_API ImageBuf {
/// as if newly constructed with the same arguments, as a read-only
/// representation of an existing image file.
void reset(string_view name, int subimage = 0, int miplevel = 0,
ImageCache* imagecache = nullptr,
const ImageSpec* config = nullptr,
Filesystem::IOProxy* ioproxy = nullptr);
std::shared_ptr<ImageCache> imagecache = {},
const ImageSpec* config = nullptr,
Filesystem::IOProxy* ioproxy = nullptr);

/// Destroy any previous contents of the ImageBuf and re-initialize it
/// as if newly constructed with the same arguments, as a read/write
Expand Down Expand Up @@ -1005,9 +1005,9 @@ class OIIO_API ImageBuf {
/// image being in RAM somewhere?
bool cachedpixels() const;

/// A pointer to the underlying ImageCache, or nullptr if this ImageBuf
/// is not backed by an ImageCache.
ImageCache* imagecache() const;
/// A shared pointer to the underlying ImageCache, or empty if this
/// ImageBuf is not backed by an ImageCache.
std::shared_ptr<ImageCache> imagecache() const;

/// Return the address where pixel `(x,y,z)`, channel `ch`, is stored in
/// the image buffer. Use with extreme caution! Will return `nullptr`
Expand Down Expand Up @@ -1151,7 +1151,7 @@ class OIIO_API ImageBuf {
// Deprecated things -- might be removed at any time

OIIO_DEPRECATED("Use `ImageBuf(name, 0, 0, imagecache, nullptr)` (2.2)")
ImageBuf(string_view name, ImageCache* imagecache)
ImageBuf(string_view name, std::shared_ptr<ImageCache> imagecache)
: ImageBuf(name, 0, 0, imagecache)
{
}
Expand All @@ -1164,7 +1164,7 @@ class OIIO_API ImageBuf {
}

OIIO_DEPRECATED("Use `reset(name, 0, 0, imagecache)` (2.2)")
void reset(string_view name, ImageCache* imagecache)
void reset(string_view name, std::shared_ptr<ImageCache> imagecache)
{
reset(name, 0, 0, imagecache);
}
Expand Down
26 changes: 13 additions & 13 deletions src/include/OpenImageIO/imagecache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
// Does invalidate() support the optional `force` flag?
#define OIIO_IMAGECACHE_INVALIDATE_FORCE 1

// Does ImageCache::create() return a shared pointer?
#define OIIO_IMAGECACHE_CREATE_SHARED 1



OIIO_NAMESPACE_BEGIN
Expand Down Expand Up @@ -56,8 +59,7 @@ class OIIO_API ImageCache {
/// or destroy the concrete implementation, so two static methods of
/// ImageCache are provided:

/// Create a ImageCache and return a raw pointer to it. This should
/// only be freed by passing it to `ImageCache::destroy()`!
/// Create a ImageCache and return a shared pointer to it.
///
/// @param shared
/// If `true`, the pointer returned will be a shared ImageCache (so
Expand All @@ -66,30 +68,28 @@ class OIIO_API ImageCache {
/// completely unique ImageCache will be created and returned.
///
/// @returns
/// A raw pointer to an ImageCache, which can only be freed with
/// A shared pointer to an ImageCache, which can only be freed with
/// `ImageCache::destroy()`.
///
/// @see ImageCache::destroy
static ImageCache* create(bool shared = true);
static std::shared_ptr<ImageCache> create(bool shared = true);

/// Destroy an allocated ImageCache, including freeing all system
/// resources that it holds.
///
/// It is safe to destroy even a shared ImageCache, as the implementation
/// of `destroy()` will recognize a shared one and only truly release
/// its resources if it has been requested to be destroyed as many times as
/// shared ImageCache's were created.
/// Release the shared_ptr to an ImageCache, including freeing all
/// system resources that it holds if no one else is still using it. This
/// is not strictly necessary to call, simply destroying the shared_ptr
/// will do the same thing, but this call is for backward compatibility
/// and is helpful if you want to use the teardown option.
///
/// @param cache
/// Raw pointer to the ImageCache to destroy.
/// Shared pointer to the ImageCache to destroy.
///
/// @param teardown
/// For a shared ImageCache, if the `teardown` parameter is
/// `true`, it will try to truly destroy the shared cache if
/// nobody else is still holding a reference (otherwise, it will
/// leave it intact). This parameter has no effect if `cache` was
/// not the single globally shared ImageCache.
static void destroy(ImageCache* cache, bool teardown = false);
static void destroy(std::shared_ptr<ImageCache>& cache, bool teardown = false);

/// @}

Expand Down
38 changes: 19 additions & 19 deletions src/include/OpenImageIO/texture.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
#define OIIO_TEXTURESYSTEM_SUPPORTS_STOCHASTIC 1
#define OIIO_TEXTURESYSTEM_SUPPORTS_DECODE_BY_USTRINGHASH 1

// Does TextureSystem::create() return a shared pointer?
#define OIIO_TEXTURESYSTEM_CREATE_SHARED 1

#ifndef INCLUDED_IMATHVEC_H
// Placeholder declaration for Imath::V3f if no Imath headers have been
// included.
Expand Down Expand Up @@ -365,8 +368,7 @@ class OIIO_API TextureSystem {
/// or destroy the concrete implementation, so two static methods of
/// TextureSystem are provided:

/// Create a TextureSystem and return a pointer to it. This should only
/// be freed by passing it to TextureSystem::destroy()!
/// Create a TextureSystem and return a shared pointer to it.
///
/// @param shared
/// If `shared` is `true`, the pointer returned will be a shared
Expand All @@ -376,41 +378,39 @@ class OIIO_API TextureSystem {
/// completely unique TextureCache will be created and returned.
///
/// @param imagecache
/// If `shared` is `false` and `imagecache` is not `nullptr`, the
/// If `shared` is `false` and `imagecache` is not empty, the
/// TextureSystem will use this as its underlying ImageCache. In
/// that case, it is the caller who is responsible for eventually
/// freeing the ImageCache after the TextureSystem is destroyed. If
/// `shared` is `false` and `imagecache` is `nullptr`, then a custom
/// `shared` is `false` and `imagecache` is empty, then a custom
/// ImageCache will be created, owned by the TextureSystem, and
/// automatically freed when the TS destroys.
///
/// @returns
/// A raw pointer to a TextureSystem, which can only be freed with
/// `TextureSystem::destroy()`.
/// A shared pointer to a TextureSystem which will be destroyed only
/// when the last shared_ptr to it is destroyed.
///
/// @see TextureSystem::destroy
static TextureSystem *create (bool shared=true,
ImageCache *imagecache=nullptr);
static std::shared_ptr<TextureSystem> create(bool shared=true,
std::shared_ptr<ImageCache> imagecache = {});

/// Destroy an allocated TextureSystem, including freeing all system
/// resources that it holds.
///
/// It is safe to destroy even a shared TextureSystem, as the
/// implementation of `destroy()` will recognize a shared one and only
/// truly release its resources if it has been requested to be destroyed
/// as many times as shared TextureSystem's were created.
/// Release the shared_ptr to a TextureSystem, including freeing all
/// system resources that it holds if no one else is still using it. This
/// is not strictly necessary to call, simply destroying the shared_ptr
/// will do the same thing, but this call is for backward compatibility
/// and is helpful if you want to use the teardown_imagecache option.
///
/// @param ts
/// Raw pointer to the TextureSystem to destroy.
/// Shared pointer to the TextureSystem to destroy.
///
/// @param teardown_imagecache
/// For a shared TextureSystem, if the `teardown_imagecache`
/// parameter is `true`, it will try to truly destroy the shared
/// cache if nobody else is still holding a reference (otherwise, it
/// will leave it intact). This parameter has no effect if `ts` was
/// not the single globally shared TextureSystem.
static void destroy (TextureSystem *ts,
bool teardown_imagecache = false);
static void destroy(std::shared_ptr<TextureSystem>& ts,
bool teardown_imagecache = false);

/// @}

Expand Down Expand Up @@ -1652,7 +1652,7 @@ class OIIO_API TextureSystem {

/// Return an opaque, non-owning pointer to the underlying ImageCache
/// (if there is one).
virtual ImageCache *imagecache () const = 0;
virtual std::shared_ptr<ImageCache> imagecache() const = 0;

virtual ~TextureSystem () { }

Expand Down
2 changes: 1 addition & 1 deletion src/iv/imageviewer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -791,7 +791,7 @@ ImageViewer::readSettings(bool ui_is_set_up)

OIIO::attribute("imagebuf:use_imagecache", 1);

ImageCache* imagecache = ImageCache::create(true);
auto imagecache = ImageCache::create(true);
imagecache->attribute("automip", autoMipmap->isChecked());
imagecache->attribute("max_memory_MB", (float)maxMemoryIC->value());
}
Expand Down
2 changes: 1 addition & 1 deletion src/iv/ivmain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ main(int argc, char* argv[])
mainWin->show();

// Set up the imagecache with parameters that make sense for iv
ImageCache* imagecache = ImageCache::create(true);
auto imagecache = ImageCache::create(true);
imagecache->attribute("autotile", 256);
imagecache->attribute("deduplicate", (int)0);
if (ap["no-autopremult"].get<int>())
Expand Down
Loading
Loading