irods / irods

Open Source Data Management Software
https://irods.org
BSD 3-Clause "New" or "Revised" License
445 stars 141 forks source link

Evaluate `std::transform` usage in query construction for replication API permissions check #7832

Open alanking opened 2 months ago

alanking commented 2 months ago

_Originally posted by @FifthPotato in https://github.com/irods/irods/pull/7827#discussion_r1648364721_

Please evaluate the pros and cons of different approaches to constructing the query used in the pre-flight permissions check for the replication API.

This check could also be made more generic and pulled out into a library for usage elsewhere in the codebase, increasing the need for a performance evaluation.

FifthPotato commented 2 months ago

Throwing in what I found here (modified from my comment on the original PR...)

groups_with_quotes.reserve(groups.size());
std::transform(std::cbegin(groups), std::cend(groups), std::back_inserter(groups_with_quotes), [](const auto& val) {
  return fmt::format("'{}'", val);
});

You have to do the back_inserter, because reserve() doesn't actually construct the object: resize() will so it works fine. If your type isn't trivially copyable, just using reserve() could segfault because it could call a custom operator= on something. If you do manage to get it to work without a back_inserter, another issue pops up: since you modified the memory directly, .size() == 0 even though it actually stores N objects. I did some local benchmarks, and just using resize() ended up being faster with no optimizations, but this one was slightly faster with -O2, so make of that what you will. This was also on a vector of 2 billion strings, and repeated 50 times, and the difference in time was on the order of a few hundred ms, so...I think the takeaway is use whatever you can remember better.

korydraughn commented 2 months ago

Make it work and then make it good/fast. That's how it goes :-)