Replaced utstring with std::string

[pschafhalter]

No description provided.

[AmplabJenkins]

Merged build finished. Test PASSed.

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/1921/
Test PASSed.

[robertnishihara]

Does it make sense to go further and make client_key a std::string as well? And then to make conn->ip_addr_port a std::string? What do you think?

[pschafhalter]

Yes, that makes sense. I'll make those updates as well.

[pcmoritz]

In fact client_key should be a const std::string& :)

[pschafhalter]

Looks like ClientConnection is still using the UT_hash data structure which can't use std::string as a key. I'll need to update that to an ordered map as well. Would you prefer I do that in a separate PR or can I make these changes in here as well?

[pcmoritz]

Keeping PRs as small as possible is best, why don't you make a separate one?

[concretevitamin]

Side note: are the changed functions on a performance-critical path? Just wondering if string concat. should avoid the + operator.

[robertnishihara]

@concretevitamin Yeah, that's a really good point.. get_manager_connection is called very frequently, so this could be an issue.

[pschafhalter]

@concretevitamin @robertnishihara is the expected length of the final string known? Calling reserve() could reduce overhead from concatenation.

Otherwise, something like std::ostringstream could also speed up concatenation.

[mehrdadn]

@concretevitamin @pschafhalter [cc @robertnishihara]
Note that it may be risky trying to spend time to make that kind of optimization without profiling. For example:

• Small-string optimization in C++ could in fact render this bottleneck a non-issue, depending on your standard library. The longest [IPv4] string here would be 21 characters (111.111.111.111:11111) and on a 64-bit system libc++ has stack space for 22 chars, though libstdc++ has 15). And the latter would still optimize out allocations for common cases such as 192.168.1.255:, reducing it to 1 actual heap allocation to construct the full address.
• If concatenation of a fixed number of strings truly does become a bottleneck, you will most likely have to remove all string instantiations (e.g. sprintf to the stack) entirely, not merely remove the concatenations.
• If you use something like std::stringstream, it will likely be slower.
• If you use sprintf, you'll have to worry about stack buffer overflows.
• Depending on your standard library, std::to_string in fact may end up being a bottleneck in itself, due to localization issues (locales/facets/etc. often end up being extremely slow).

Generally, I would avoid spending time on such optimizations unless (a) the optimized version isn't too much harder to write (& read) than the original version, and (b) unless it is completely obvious that the optimized version will lack the bottlenecks. Otherwise, it's best to just wait until a profiler pinpoints potential hotspots.

[robertnishihara]

Thanks a lot @mehrdadn, that's super informative! And I love that the takeaway is that I don't have to do anything right now :)