Reduce number of string copies and map lookups in FakeDownloader.

Reduce number of string copies and map lookups in FakeDownloader.

R=roubert@google.com

Committed: https://code.google.com/p/libaddressinput/source/detail?r=319

[Rouslan Solomakhin, 2014-07-23 19:03:31]

Fredrik: PTAL. These test-only changes do not bring significant speed ups on
their own, but help to reduce noise in profiler and profile the rest of code
that makes map inserts and string copies.

[roubert (google), 2014-07-24 01:19:48]

https://codereview.appspot.com/117140043/diff/1/cpp/test/fake_downloader.cc
File cpp/test/fake_downloader.cc (right):

https://codereview.appspot.com/117140043/diff/1/cpp/test/fake_downloader.cc#n...
cpp/test/fake_downloader.cc:117: if (key.length() > kAggregateDataKeyLength) {
Hm, it's murky to me why this test would tell whether aggregate_data_it points
to an object to which the current data should be appended.

Is it because the input data is sorted, so that any key with length greater than
kAggregateDataKeyLength will come after the key that is should be appended to?

If so, then that seems both a bit hard to guess and a bit fragile, and it might
be better to do a slightly more costly and long-winded, but I think clearer,
prefix check instead:

        // If aggregate_data_it and key have the same prefix (eg. "data/ZZ").
        if (aggregate_data_it != data.end() &&
            key.compare(
                0,
                kAggregateDataKeyLength,
                aggregate_data_it->first,
                kFakeAggregateDataUrlLength, 
                kAggregateDataKeyLength) == 0) {

That won't crash if a sub key is placed before its parent key in the
testdata/countryinfo.txt file. ;-)

Or, if you see such a crash as a nice side-benefit, then please document that
clearly here.

[Rouslan Solomakhin, 2014-07-24 06:15:28]

Fredrik: PTAL Patch Set 2.

https://codereview.appspot.com/117140043/diff/1/cpp/test/fake_downloader.cc
File cpp/test/fake_downloader.cc (right):

https://codereview.appspot.com/117140043/diff/1/cpp/test/fake_downloader.cc#n...
cpp/test/fake_downloader.cc:117: if (key.length() > kAggregateDataKeyLength) {
On 2014/07/24 01:19:47, roubert (google) wrote:
> Hm, it's murky to me why this test would tell whether aggregate_data_it points
> to an object to which the current data should be appended.
> 
> Is it because the input data is sorted, so that any key with length greater
than
> kAggregateDataKeyLength will come after the key that is should be appended to?
> 
> If so, then that seems both a bit hard to guess and a bit fragile, and it
might
> be better to do a slightly more costly and long-winded, but I think clearer,
> prefix check instead:
> 
>         // If aggregate_data_it and key have the same prefix (eg. "data/ZZ").
>         if (aggregate_data_it != data.end() &&
>             key.compare(
>                 0,
>                 kAggregateDataKeyLength,
>                 aggregate_data_it->first,
>                 kFakeAggregateDataUrlLength, 
>                 kAggregateDataKeyLength) == 0) {
> 
> That won't crash if a sub key is placed before its parent key in the
> testdata/countryinfo.txt file. ;-)
> 
> Or, if you see such a crash as a nice side-benefit, then please document that
> clearly here.

Done.

[roubert (google), 2014-07-24 06:20:43]

LGTM

[Rouslan Solomakhin, 2014-07-24 07:48:12]

Committed patchset #2 manually as r319 (presubmit successful).