Skip to content

C++ interfaces: Write directly to std::string#6425

Open
abarton5 wants to merge 1 commit into
HDFGroup:developfrom
abarton5:cppAloc
Open

C++ interfaces: Write directly to std::string#6425
abarton5 wants to merge 1 commit into
HDFGroup:developfrom
abarton5:cppAloc

Conversation

@abarton5

Copy link
Copy Markdown

Since C++11 std::string has been guaranteed to use a contiguous internal buffer. We can therefore write directly to internal memory. This eliminates the need for temporary allocations and in short string cases will eliminate all calls to malloc due to short string optimization.

Comment thread c++/src/H5Attribute.cpp
}else{
strg.resize(attr_size);
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The proposed changes are technically valid but trade code simplicity for marginal (likely zero) performance gain.

Allocation: This does not actually eliminate heap allocation; it simply moves it from an explicit new[] to the implicit heap allocation triggered by strg.resize().
Complexity & Risk: Bypassing standard string assignment forces us to manually reinvent C-string truncation logic (strg.find('\0')), increasing code complexity and correctness risk.

My recommendation is to leave the original code alone, or modernize it safely using a standard RAII std::vector buffer to cleanly eliminate the manual delete[] calls without adding truncation complexity.

@github-project-automation github-project-automation Bot moved this from To be triaged to In progress in HDF5 - TRIAGE & TRACK Jun 2, 2026
@abarton5

abarton5 commented Jun 2, 2026

Copy link
Copy Markdown
Author

In the case of short strings there are no allocation. It also reduces the copying,
Here is a benchmark if you want to test the claim.
image

#include <benchmark/benchmark.h>
#include <string>
#include <cstring>

// Simulate the HDF5 C API writing into a buffer and null-terminating it
void simulated_c_api(char* buffer, size_t size) {
    std::memset(buffer, 'A', size);
    buffer[size] = '\0'; 
}

// Old Method: Manual new/delete with C-buffer
static void BM_CBufferMethod(benchmark::State& state) {
    size_t size = state.range(0);
    for (auto _ : state) {
        // 1. Allocate C buffer on the heap (zero-initialized)
        char* c_buf = new char[size + 1]();
        
        // 2. Call simulated C API
        simulated_c_api(c_buf, size);
        
        // 3. Copy to std::string
        std::string result(c_buf, size);
        
        // 4. Free C buffer
        delete[] c_buf;
        
        // Prevent compiler from optimizing the loop away
        benchmark::DoNotOptimize(result);
    }
}

// C++11 Method: Direct write to std::string (From your patch)
static void BM_DirectStringMethod(benchmark::State& state) {
    size_t size = state.range(0);
    for (auto _ : state) {
        std::string result;
        
        // 1. Resize std::string (forces value-initialization/zero-filling)
        result.resize(size + 1);
        
        // 2. Call simulated C API directly into the buffer
        simulated_c_api(&result[0], size);
        
        // 3. Truncate to remove the null terminator
        result.resize(size);
        
        benchmark::DoNotOptimize(result);
    }
}


// Test varying sizes: from Small String Optimization (SSO) sizes up to 8KB
BENCHMARK(BM_CBufferMethod)->Range(4, 30);
BENCHMARK(BM_DirectStringMethod)->Range(4, 30);

you can of course judge if it is worth the added complexity

@bmribler

bmribler commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Thanks for providing the benchmark! I acknowledge that the performance improvement is real. I understand that for names ≤ ~15–23 bytes (SSO threshold), all heap allocations are eliminated and for longer names, the allocation count reduces from two to one. However, unless string name operations are demonstrably in the hot path for a given workload, the performance gain does not justify the complexity and the correctness risk for production library code. That said, I revise my recommendation toward the vector approach. It produces no find('\0') logic and no truncation complexity. It uses assign(data, len) or assign(char*), both of which have well-understood, correct semantics.

// For example, for Attribute::p_read_fixed_len
std::vector buf(attr_size + 1, '\0');
H5Aread(id, mem_type.getId(), buf.data());
strg.assign(buf.data()); // strlen-based assign stops at first '\0'

As a last note, we are discussing about retiring the HDF5 C++ API as there are more modern third-party alternatives available, such as HighFive, h5cpp, h5xx.

@lrknox lrknox removed their request for review June 11, 2026 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

2 participants