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

Jump to bottom

Remove redundant instructions from tile hash #3898

Merged
merged 7 commits into from
Jul 25, 2023
Merged

Remove redundant instructions from tile hash #3898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
faaf216
Remove redundant instructions from hash
curtisblack Jun 26, 2023
98f4198
Ignore false warning about uninitialized tile data.
curtisblack Jul 21, 2023
e439abb
Remove redundant instructions from hash
curtisblack Jun 26, 2023
b9a57e0
Ignore false warning about uninitialized tile data.
curtisblack Jul 21, 2023
800bec7
Merge branch 'optimize_tile_hash' of https://github.com/curtisblack/o…
curtisblack Jul 23, 2023
36ec5b3
Move compiler ignore directives.
curtisblack Jul 24, 2023
789406c
Fix false positive warning about uninitialized memory in gcc11
lgritz Jul 24, 2023
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: 5 additions & 0 deletions src/include/OpenImageIO/hash.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,12 @@ inline uint64_t fasthash64(const void *buf, size_t len, uint64_t seed=1771)
uint64_t v;

while (pos != end) {
// This appears to be a false positive which only affects GCC.
// https://godbolt.org/z/5q7Y7ndfb
OIIO_PRAGMA_WARNING_PUSH
OIIO_GCC_ONLY_PRAGMA(GCC diagnostic ignored "-Wmaybe-uninitialized")
v = *pos++;
OIIO_PRAGMA_WARNING_PUSH
h ^= mix(v);
h *= m;
}
Expand Down
19 changes: 12 additions & 7 deletions src/libtexture/imagecache_pvt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,13 +560,17 @@ struct TileID {
/// Digest the TileID into a size_t to use as a hash key.
size_t hash() const
{
const uint64_t a = (uint64_t(m_x) << 32) + uint64_t(m_y);
const uint64_t b = (uint64_t(m_z) << 32) + uint64_t(m_subimage);
const uint64_t c = (uint64_t(m_miplevel) << 32)
+ (uint64_t(m_chbegin) << 16) + uint64_t(m_chend);
const uint64_t d = m_file->filename().hash()
+ uint64_t(m_colortransformid);
return fasthash::fasthash64({ a, b, c, d });
static constexpr size_t member_size
= sizeof(m_x) + sizeof(m_y) + sizeof(m_z) + sizeof(m_subimage)
+ sizeof(m_miplevel) + sizeof(m_chbegin) + sizeof(m_chend)
+ sizeof(m_colortransformid) + sizeof(m_padding) + sizeof(m_file);
static_assert(
sizeof(*this) == member_size,
"All TileID members must be accounted for so we can hash the entire class.");
static_assert(
sizeof(*this) % sizeof(uint64_t) == 0,
"FastHash uses the fewest instructions when data size is a multiple of 8 bytes.");
return fasthash::fasthash64(this, sizeof(*this));
}

/// Functor that hashes a TileID
Expand All @@ -590,6 +594,7 @@ struct TileID {
int m_miplevel; ///< MIP-map level
short m_chbegin, m_chend; ///< Channel range
int m_colortransformid; ///< Colorspace id (0 == default)
int m_padding = 0; ///< Unused
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be evaluated separately, but maybe instead of adding 32-bits of padding, m_miplevel and m_colortransformid could maybe be changed to 16-bit to shave off an extra 8 bytes?

I'd be at least curious to know if that saves any measurable amount of time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I didn't necessarily wish to bundle that type of change into this PR but I also thought something similar.

Unfortunately I don't have a build with all the recent color space additions to test performance. The performance was measured on v2.4.6, prior to the addition of m_colortransformid.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can save this for another pass, but it seems like a good idea to me.

ImageCacheFile* m_file; ///< Which ImageCacheFile we refer to
};

Expand Down
Loading