Use string view for interface labels and descriptions #1210

sethrj · 2024-04-28T13:24:03Z

This prevents repeated allocation/deallocation during the stepping loop as noticed by @esseivaju in #1172.

esseivaju

Nice! Just a few comments.

esseivaju · 2024-04-28T20:58:03Z

src/accel/LocalTransporter.cc

@@ -262,8 +262,7 @@ auto LocalTransporter::GetActionTime() const -> MapStrReal
        CELER_ASSERT(action_ptrs.size() == time.size());
        for (auto i : range(action_ptrs.size()))
        {
-            auto&& label = action_ptrs[i]->label();
-            result[label] = time[i];
+            result[std::string{action_ptrs[i]->label()}] = time[i];


We can change MapStrReal in LocalTransporter and TimerOutput to std::unordered_map<std::string_view, real_type>

I don't think we can do it for TimerOutput because that might exceed the lifetime of the core params... I figured it was safer to do a few one-time string copies than to risk it...

So far we only use string literals that have static storage, but sure in general it's less error-prone for the map to own the key. It just doesn't look nice having to instantiate a string to look up the map when the string_view has the information needed. Gotta wait for C++20 support to fix that.

Do you mean C++26? (which changes operator[])?
Or should we use unordered_map::find which is already (in C++14) templated on the parameter type?

I think you did mean C++20 as described here: https://www.cppstories.com/2021/heterogeneous-access-cpp20/. I.e. the templated function/operator is not enough, one also has to customize the hash comparisons.

Bummer, I saw that and hoped it would be automatic for the string/view case

src/celeritas/global/ActionRegistry.cc

src/corecel/io/OutputRegistry.cc

src/corecel/sys/ScopedProfiling.cuda.cc

esseivaju · 2024-04-28T21:22:22Z

test/celeritas/global/StepperTestBase.cc

@@ -72,16 +72,16 @@ auto StepperTestBase::check_setup() -> SetupCheckResult

    for (auto process_id : range(ProcessId{p.num_processes()}))
    {
-        result.processes.push_back(p.process(process_id)->label());
+        result.processes.push_back(std::string{p.process(process_id)->label()});


I think emplace_back looks better here (or vector of string_view)

esseivaju · 2024-04-28T21:24:04Z

test/celeritas/phys/Physics.test.cc

@@ -64,9 +64,9 @@ TEST_F(PhysicsParamsTest, accessors)
    std::vector<std::string> process_names;
    for (auto process_id : range(ProcessId{p.num_processes()}))
    {
-        process_names.push_back(p.process(process_id)->label());
+        process_names.push_back(std::string{p.process(process_id)->label()});


emplace_back here as well

esseivaju

I reran the profiler for the one-slot scenario, we went from 93 million calls to free to 170k so it's pretty good.

pcanal · 2024-04-29T21:10:08Z

app/celer-sim/Transporter.cc

@@ -165,8 +174,7 @@ void Transporter<M>::accum_action_times(MapStrDouble* result) const
        CELER_ASSERT(action_ptrs.size() == times.size());
        for (auto i : range(action_ptrs.size()))
        {
-            auto&& label = action_ptrs[i]->label();
-            (*result)[label] += times[i];
+            (*result)[std::string{action_ptrs[i]->label()}] += times[i];


Doesn't that increase (keep the same) the number of allocation?

Yeah, but it should only happen once per stream per run — and usually it's going directly into a JSON object which has lots of allocations and can't accept string_view anyway.

pcanal · 2024-04-29T21:18:51Z

src/accel/LocalTransporter.cc

@@ -262,8 +262,7 @@ auto LocalTransporter::GetActionTime() const -> MapStrReal
        CELER_ASSERT(action_ptrs.size() == time.size());
        for (auto i : range(action_ptrs.size()))
        {
-            auto&& label = action_ptrs[i]->label();
-            result[label] = time[i];
+            result[std::string{action_ptrs[i]->label()}] = time[i];


Do you mean C++26? (which changes operator[])?
Or should we use unordered_map::find which is already (in C++14) templated on the parameter type?

src/corecel/io/OutputRegistry.cc

pcanal · 2024-04-29T21:39:59Z

src/accel/LocalTransporter.cc

@@ -262,8 +262,7 @@ auto LocalTransporter::GetActionTime() const -> MapStrReal
        CELER_ASSERT(action_ptrs.size() == time.size());
        for (auto i : range(action_ptrs.size()))
        {
-            auto&& label = action_ptrs[i]->label();
-            result[label] = time[i];
+            result[std::string{action_ptrs[i]->label()}] = time[i];


I think you did mean C++20 as described here: https://www.cppstories.com/2021/heterogeneous-access-cpp20/. I.e. the templated function/operator is not enough, one also has to customize the hash comparisons.

sethrj · 2024-04-30T03:05:40Z

Sorry, just noticed I didn't hit "send" on this comment yesterday...

@esseivaju I think it's much too dangerous to keep long-term string views. Even this is going to lead to an invalid read:

my_string_vec.emplace_back(label);
my_string_view_map.insert({my_string_vec.back(), value});

because once the vector changes capacity, the string views in the map are invalidated.

I think string views are meant primarily for interfaces and not for long-term storage.

sethrj added core Software engineering infrastructure minor Minor internal changes or fixes performance Changes for performance optimization labels Apr 28, 2024

sethrj requested a review from esseivaju April 28, 2024 13:24

sethrj added 3 commits April 28, 2024 09:24

Use string_view instead of string for interface labels+descriptions

66e223f

Use string view in scoped profiling

a3b6bbe

Preallocate stepping counters in transport loop

9bb78d4

sethrj force-pushed the string-view-labels branch from 4d7ce62 to 9bb78d4 Compare April 28, 2024 13:25

Fix device build errors

2c44958

esseivaju reviewed Apr 28, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into string-view-labels

30f60b7

esseivaju approved these changes Apr 29, 2024

View reviewed changes

pcanal reviewed Apr 29, 2024

View reviewed changes

Incorporate review feedback

0120d74

sethrj merged commit 2767efe into celeritas-project:develop Apr 30, 2024
28 checks passed

sethrj deleted the string-view-labels branch May 1, 2024 00:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use string view for interface labels and descriptions #1210

Use string view for interface labels and descriptions #1210

sethrj commented Apr 28, 2024

esseivaju left a comment

esseivaju Apr 28, 2024

sethrj Apr 29, 2024

esseivaju Apr 29, 2024

pcanal Apr 29, 2024

pcanal Apr 29, 2024

sethrj Apr 30, 2024

esseivaju Apr 28, 2024 •

edited

Loading

esseivaju Apr 28, 2024

esseivaju left a comment

pcanal Apr 29, 2024

sethrj Apr 30, 2024

pcanal Apr 29, 2024

pcanal Apr 29, 2024

sethrj commented Apr 30, 2024

Use string view for interface labels and descriptions #1210

Use string view for interface labels and descriptions #1210

Conversation

sethrj commented Apr 28, 2024

esseivaju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esseivaju Apr 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esseivaju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethrj commented Apr 30, 2024

esseivaju Apr 28, 2024 •

edited

Loading