clang warning on exit-time declaration

[YWHo]

This code at around line 367:
inline std::string uuid::base62() const {
static const std::string base62 =
"0123456789"
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
"abcdefghijklmnopqrstuvwxyz";
return rebase( ab, base62 ) + "-" + rebase( cd, base62 );
}

and this code at around line 737:
static const std::string base62 =
"0123456789"
"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
"abcdefghijklmnopqrstuvwxyz";

shows clang warning as below:

warning: declaration requires an exit-time destructor [-Wexit-time-destructors]
static const std::string base62 =
^

The IDE I used is "Xcode Version 7.3.1"

[dan-ryan]

I'm not sure that this warning is really a problem. It's probably better to ignore it.
The fix done, now makes it non static, so it creates a char * every time.
My suggestion is to add static, constexpr and ignore "exit-time declaration" warnings for this method.

@r-lyeh

[Edensan]

@r-lyeh Was that really a fix though?
As you mentioned yourself this was probably better ignored as it deals with trivial objects (safe to destroy on-exit, regardless of order).

This is a "run-time" vs "on-exit" performance decision, and in my experience most people would gladly accept the trade-off.

After the "fix", at a first glance the performance seems atrocious, a single call of uuid::base62():

• Creates a new (const char*) string
• Creates a new std::string for each rebase(...)
• Each std::string makes a copy of the newly created (const char*) string

So for example, the base62() method will allocate the string 3 times where just a reference to the static std::string would have been enough.

In applications where objects and their uuid's need to be serialized, this looks unacceptable.

[r-lyeh-archived]

Have anyone measured/benchmarked both implementations before blaming at the fix? :)

I guess the performance is similar in both cases: it was 1 global string access + 3 string allocs before, and it is a local access (to a likely inlined variable) + 3 string allocs now. But then again, it would need some benchmarking before taking any further decision.

It could be better if I would have done a std::string(base62, sizeof(base62)/sizeof(base62[0])) instead, though. That's true.

[dan-ryan]

Months ago I did do some benchmarks to improve performance in our application. UUID creation was the slowest part, the optimisations I suggested above seemed to help improve it, at least in my benchmarks.

[Edensan]

Quickly benchmarking calls to base62() revealed a 10~15% performance difference on my computer.

Locally allocated results
$ g++ sole.cxx -std=c++11 && ./a.out
Benchmarking... 263582 base62/sec
Benchmarking... 268503 base62/sec
Benchmarking... 267937 base62/sec
Benchmarking... 258611 base62/sec
Benchmarking... 266666 base62/sec
Benchmarking... 267978 base62/sec
Benchmarking... 266684 base62/sec
Benchmarking... 248146 base62/sec
Benchmarking... 250766 base62/sec

Cached std::string results
$ g++ sole.cxx -std=c++11 && ./a.out
Benchmarking... 286939 base62/sec
Benchmarking... 284413 base62/sec
Benchmarking... 283041 base62/sec
Benchmarking... 290042 base62/sec
Benchmarking... 289638 base62/sec
Benchmarking... 285924 base62/sec
Benchmarking... 286872 base62/sec
Benchmarking... 292314 base62/sec
Benchmarking... 284472 base62/sec

[Edensan]

I guess the performance is similar in both cases: it was 1 global string access + 3 string allocs before, and it is a local access (to a likely inlined variable) + 3 string allocs now

Uuuh, what? I have a feeling we are not talking about the same thing.

rebase( ..., const std::string &basemap )
This requires a std::string as a parameter, which means a cast/conversion to std::string is required for this to even compile. AFAIK new std::string instances will re-allocate the original string and keep their own internal copy.

If before we were accessing a global std::string instance and feeding it to the rebase, no casts were being required and thus removing the need for unnecessary string allocations.

[Edensan]

Just found something else inside rebase,
From: res = std::string() + basemap[int(rem)] + res;
To: res = basemap[int(rem)] + res;

Results:
$ g++ sole.cxx -std=c++11 && ./a.out
Benchmarking... 325883 base62/sec
Benchmarking... 328972 base62/sec
Benchmarking... 326594 base62/sec
Benchmarking... 333665 base62/sec
Benchmarking... 334059 base62/sec
Benchmarking... 312550 base62/sec
Benchmarking... 328770 base62/sec
Benchmarking... 334422 base62/sec
Benchmarking... 327754 base62/sec

Another ~15% on top of the previous benchmark (with the cached std::string).

[dan-ryan]

Great find @Edensan. I might have to do that quick edit for our app.

[r-lyeh-archived]

Super. I 'll have a commit to this fix asap, unless somebody else is faster at PRs :)
Thanks @Edensan to take the time to measure it

[r-lyeh-archived]

@Edensan can you benchmark that branch please? I'm curious to see the results!

[Edensan]

Benchmarking... 1902357 base62/sec
Benchmarking... 1986467 base62/sec
Benchmarking... 2000317 base62/sec
Benchmarking... 2019920 base62/sec
Benchmarking... 1984543 base62/sec
Benchmarking... 1915283 base62/sec

Woah, almost more than 6x performance improvement for the base62() apparently, that's what I call an optimization 👍

[Edensan]

My guess is that re-allocating strings in that loop was just killing the performance, I see you're using a char buffer now and doing a single std::string allocation, good.

[dan-ryan]

What an increase! Nice work @r-lyeh
And that's without const char base62[] being static.

[r-lyeh-archived]

I wonder if base62 remains 100% intact and thus is backward compatible. I am 98% sure it is safe to use. It would be great if any of you guys mind to apply the branch and test your unit-test suites with it :)

[dan-ryan]

@r-lyeh Got a warning that this line is not being used.

					const char base62[] =
							"0123456789"
									"ABCDEFGHIJKLMNOPQRSTUVWXYZ"
									"abcdefghijklmnopqrstuvwxyz";

But all my tests run fine. Performance is looking good. Time to merge the changes into master after fixing this warning?

[r-lyeh-archived]

hey @zammbi, I just checked and the base62optim branch wont pass tests. Gotta review the rebuild(b62) method and let you know then
edit: it's base62() actually

[r-lyeh-archived]

Merged