Skip to content

Enhance implementation of ShardedMap class#59

Open
missa-prime wants to merge 7 commits into
intel:mainfrom
missa-prime:user/missa-prime/concurrency
Open

Enhance implementation of ShardedMap class#59
missa-prime wants to merge 7 commits into
intel:mainfrom
missa-prime:user/missa-prime/concurrency

Conversation

@missa-prime

@missa-prime missa-prime commented Jun 29, 2026

Copy link
Copy Markdown

A summary of the code changes in this PR is provided below.

  1. Add a new scheme in ShardedMap that uses concurrent hash maps from oneTBB.
  2. Enable the new scheme if QAT, IAA, or IGZIP is used.
  3. Use Fibonacci hashing instead of modulo to generate shard indexes from stream keys.
  4. Expose the number of map shards as a configuration parameter.
  5. Ensure the each map instance is 64-byte aligned to mitigate the effects of false sharing within cache lines.

With these modifications, the shim layer now has the option of using an optimized thread safe map data structure provided by oneTBB instead of one composed with a shared mutex and an unordered map. Additionally, the switch to Fibonacci hashing means that threads are less likely to collide on the map shard indexes.

Finally, no additional test failures were observed.

@asonje

asonje commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

sharded_map.h line 9 — TBB header portability

#include <oneapi/tbb/concurrent_hash_map.h> and oneapi::tbb:: are only available in oneTBB 2021.x+. Systems with older Intel TBB (Ubuntu 20.04 ships libtbb-dev 2020.3, RHEL 8 similar) will fail to build. tbb/concurrent_hash_map.h and the tbb:: namespace work on both old and new TBB — oneTBB provides the tbb/ header as a compatibility wrapper, so tbb::concurrent_hash_map is an alias for oneapi::tbb::concurrent_hash_map on modern installs. Suggest:

#include <tbb/concurrent_hash_map.h>
using MapType = tbb::concurrent_hash_map<Key, Value, HashCompare>;

@asonje

asonje commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

CMakeLists.txt lines 11–12 — TBB as unconditional dependency

Making TBB unconditionally required (find_package(TBB REQUIRED) in the root CMakeLists.txt) means libtbb.so becomes a runtime dependency of libzlib-accel.so even for builds with no hardware acceleration enabled. This differs from how QATzip, QPL, and ISA-L are handled — those only link when their respective USE_QAT / USE_IAA / USE_IGZIP flag is set.

Suggest auto-enabling TBB when any hardware path is active (since ShardedMap is on the hot path for all streams, and users enabling hardware are already in Intel oneAPI territory where TBB is available):

# in common.cmake, after USE_QAT/USE_IAA/USE_IGZIP blocks
if(USE_QAT OR USE_IAA OR USE_IGZIP)
  find_package(TBB REQUIRED COMPONENTS tbb)
  link_libraries(TBB::tbb)
  add_compile_definitions(USE_TBB)
endif()

Then guard sharded_map.h with #ifdef USE_TBB, falling back to the existing std::shared_mutex implementation (enhanced with Fibonacci hashing and alignas(64)) when TBB is absent. This keeps pure-zlib builds dependency-free.

@asonje

asonje commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

sharded_map.h line 15 — using namespace config; in a header

using namespace config; at file scope in a header pollutes the global namespace in every translation unit that includes sharded_map.h. Suggest replacing with a qualified reference to the config value: pass num_shards as a constructor parameter, or use config::configs[config::MAP_SHARDS] directly.

@asonje

asonje commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

sharded_map.h GetShardnum_shards re-read on every call

GetShard reads configs[MAP_SHARDS] on every invocation, but the array was sized once in the constructor. If the config value diverged after construction (e.g. in a test that calls SetConfig), GetShard would produce an out-of-bounds shard index silently. Suggest caching it as a const member set in the constructor:

const unsigned int num_shards_;
// constructor: num_shards_(configs[MAP_SHARDS])

Then GetShard references num_shards_ rather than calling into configs[].

@asonje

asonje commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

config/config.cpp ~line 92 — Power-of-2 not validated at runtime

trySetConfig(MAP_SHARDS, 65536, 1) accepts any value in [1, 65536], but GetShard requires a power of 2 (uses __builtin_ctz for the shift). The assert in GetShard is compiled away by NDEBUG in Release builds — a user who sets map_shards=100 gets a silently skewed distribution (only 4 shards used out of 100) with no error. Suggest validating at config load time:

trySetConfig(MAP_SHARDS, 65536, 1);
if (configs[MAP_SHARDS] & (configs[MAP_SHARDS] - 1)) {
    Log(LogLevel::LOG_ERROR,
        "map_shards must be a power of 2, ignoring value ",
        configs[MAP_SHARDS], ", using default 64\n");
    configs[MAP_SHARDS] = 64;
}

@asonje

asonje commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

CMakeLists.txt lines 11–12 — CI failure / unconditional TBB

This is the root cause of all three CI check failures. The workflow installs build-essential, clang, cmake, libgtest-dev, zlib1g-dev — no libtbb-dev — so find_package(TBB REQUIRED) fails at cmake configure and the build dies in ~21 seconds before any code compiles.

Adding libtbb-dev to the workflow would not fully fix this either, because Ubuntu's apt repo ships the old TBB 2020.x package, and #include <oneapi/tbb/concurrent_hash_map.h> does not exist in that version (see separate comment on sharded_map.h).

The deeper issue is that TBB is now a hard runtime dependency of libzlib-accel.so unconditionally, even for builds with no hardware acceleration enabled. All other library dependencies (QATzip, QPL, ISA-L) only link when their respective hardware flag is set.

Suggest: auto-enable TBB when any hardware path is active. Since ShardedMap is on the hot path of every deflate()/inflate() call, and users enabling hardware acceleration are already in Intel oneAPI territory where TBB is available, this is a natural coupling. Keep a stdlib fallback (std::shared_mutex + std::unordered_map, enhanced with Fibonacci hashing and alignas(64)) for the no-hardware case:

In common.cmake:

# ShardedMap uses TBB when any hardware acceleration path is enabled.
# Users building with hardware support are already in Intel oneAPI territory.
if(USE_QAT OR USE_IAA OR USE_IGZIP)
  find_package(TBB REQUIRED COMPONENTS tbb)
  link_libraries(TBB::tbb)
  add_compile_definitions(USE_TBB)
endif()

In sharded_map.h:

#ifdef USE_TBB
  #include <tbb/concurrent_hash_map.h>  // portable: old TBB + oneTBB
  // tbb::concurrent_hash_map implementation
#else
  #include <shared_mutex>
  #include <unordered_map>
  // stdlib implementation with Fibonacci hashing + alignas(64)
#endif

With this change the CI build (no hardware flags) passes with zero workflow changes. A separate CI job with libtbb-dev and a hardware flag set would be needed for TBB path coverage.> # ShardedMap uses TBB when any hardware acceleration path is enabled.

Users building with hardware support are already in Intel oneAPI territory.

if(USE_QAT OR USE_IAA OR USE_IGZIP)
find_package(TBB REQUIRED COMPONENTS tbb)
link_libraries(TBB::tbb)
add_compile_definitions(USE_TBB)
endif()


In `sharded_map.h`:
```cpp
#ifdef USE_TBB
  #include <tbb/concurrent_hash_map.h>  // portable: old TBB + oneTBB
  // tbb::concurrent_hash_map implementation
#else
  #include <shared_mutex>
  #include <unordered_map>
  // stdlib implementation with Fibonacci hashing + alignas(64)
#endif

With this change the CI build (no hardware flags) passes with zero workflow changes. A separate CI job with libtbb-dev and a hardware flag set would be needed for TBB path coverage.

@missa-prime

Copy link
Copy Markdown
Author

config/config.cpp ~line 92 — Power-of-2 not validated at runtime

trySetConfig(MAP_SHARDS, 65536, 1) accepts any value in [1, 65536], but GetShard requires a power of 2 (uses __builtin_ctz for the shift). The assert in GetShard is compiled away by NDEBUG in Release builds — a user who sets map_shards=100 gets a silently skewed distribution (only 4 shards used out of 100) with no error. Suggest validating at config load time:

trySetConfig(MAP_SHARDS, 65536, 1);
if (configs[MAP_SHARDS] & (configs[MAP_SHARDS] - 1)) {
    Log(LogLevel::LOG_ERROR,
        "map_shards must be a power of 2, ignoring value ",
        configs[MAP_SHARDS], ", using default 64\n");
    configs[MAP_SHARDS] = 64;
}

I opted to make this case unrecoverable by returning false instead. I think the user should be fully aware what value is being used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants