From bc335343da0b0364f5245ae270ffdc493e818322 Mon Sep 17 00:00:00 2001 From: Rouslan Solomakhin Date: Thu, 10 Jul 2014 17:58:38 +0000 Subject: Avoid string copying in json.cc. The following two methods: bool Json::HasStringValueForKey(const std::string& key) const; std::string Json::GetStringValueForKey(const std::string& key) const; have been replaced with a single method: bool Json::GetStringValueForKey(const std::string& key, std::string* value) const; to avoid copying possibly large strings. BUG=8 R=roubert@google.com Review URL: https://codereview.appspot.com/112940044 --- cpp/src/preload_supplier.cc | 4 +-- cpp/src/rule.cc | 77 +++++++++++++++++---------------------------- cpp/src/util/json.cc | 22 ++++++------- cpp/src/util/json.h | 14 +++------ cpp/test/util/json_test.cc | 42 +++++++++++++++---------- 5 files changed, 70 insertions(+), 89 deletions(-) (limited to 'cpp') diff --git a/cpp/src/preload_supplier.cc b/cpp/src/preload_supplier.cc index 31325bd..c613d3c 100644 --- a/cpp/src/preload_supplier.cc +++ b/cpp/src/preload_supplier.cc @@ -105,6 +105,7 @@ class Helper { (void)status; // Prevent unused variable if assert() is optimized away. Json json; + std::string id; std::vector sub_rules; if (!success) { @@ -124,11 +125,10 @@ class Helper { } const Json& value = json.GetDictionaryValueForKey(*it); - if (!value.HasStringValueForKey("id")) { + if (!value.GetStringValueForKey("id", &id)) { success = false; goto callback; } - const std::string& id = value.GetStringValueForKey("id"); assert(*it == id); // Sanity check. size_t depth = std::count(id.begin(), id.end(), '/') - 1; diff --git a/cpp/src/rule.cc b/cpp/src/rule.cc index b4d03ca..4d1cff5 100644 --- a/cpp/src/rule.cc +++ b/cpp/src/rule.cc @@ -38,20 +38,6 @@ namespace { typedef std::map NameMessageIdMap; -const char kAdminAreaNameTypeKey[] = "state_name_type"; -const char kFormatKey[] = "fmt"; -const char kIdKey[] = "id"; -const char kLanguagesKey[] = "languages"; -const char kLatinFormatKey[] = "lfmt"; -const char kLatinNameKey[] = "lname"; -const char kNameKey[] = "name"; -const char kPostalCodeNameTypeKey[] = "zip_name_type"; -const char kRequireKey[] = "require"; -const char kSubKeysKey[] = "sub_keys"; -const char kZipKey[] = "zip"; -const char kPostalCodeExampleKey[] = "zipex"; -const char kPostServiceUrlKey[] = "posturl"; - // Used as a separator in a list of items. For example, the list of supported // languages can be "de~fr~it". const char kSeparator = '~'; @@ -182,36 +168,33 @@ bool Rule::ParseSerializedRule(const std::string& serialized_rule) { } void Rule::ParseJsonRule(const Json& json) { - if (json.HasStringValueForKey(kIdKey)) { - id_ = json.GetStringValueForKey(kIdKey); + std::string value; + if (json.GetStringValueForKey("id", &value)) { + id_.swap(value); } - if (json.HasStringValueForKey(kFormatKey)) { - ParseFormatRule(json.GetStringValueForKey(kFormatKey), &format_); + if (json.GetStringValueForKey("fmt", &value)) { + ParseFormatRule(value, &format_); } - if (json.HasStringValueForKey(kLatinFormatKey)) { - ParseFormatRule(json.GetStringValueForKey(kLatinFormatKey), &latin_format_); + if (json.GetStringValueForKey("lfmt", &value)) { + ParseFormatRule(value, &latin_format_); } - if (json.HasStringValueForKey(kRequireKey)) { - ParseAddressFieldsRequired( - json.GetStringValueForKey(kRequireKey), &required_); + if (json.GetStringValueForKey("require", &value)) { + ParseAddressFieldsRequired(value, &required_); } - if (json.HasStringValueForKey(kSubKeysKey)) { - SplitString( - json.GetStringValueForKey(kSubKeysKey), kSeparator, &sub_keys_); + if (json.GetStringValueForKey("sub_keys", &value)) { + SplitString(value, kSeparator, &sub_keys_); } - if (json.HasStringValueForKey(kLanguagesKey)) { - SplitString( - json.GetStringValueForKey(kLanguagesKey), kSeparator, &languages_); + if (json.GetStringValueForKey("languages", &value)) { + SplitString(value, kSeparator, &languages_); } sole_postal_code_.clear(); - if (json.HasStringValueForKey(kZipKey)) { - const std::string& zip = json.GetStringValueForKey(kZipKey); + if (json.GetStringValueForKey("zip", &value)) { // The "zip" field in the JSON data is used in two different ways to // validate the postal code. At the country level, the "zip" field indicates // a Java compatible regular expression corresponding to all postal codes in @@ -225,7 +208,7 @@ void Rule::ParseJsonRule(const Json& json) { // RE2::FullMatch() to perform matching against the entire string. RE2::Options options; options.set_never_capture(true); - RE2* matcher = new RE2("^(" + zip + ")", options); + RE2* matcher = new RE2("^(" + value + ")", options); if (matcher->ok()) { postal_code_matcher_.reset(new RE2ptr(matcher)); } else { @@ -234,37 +217,35 @@ void Rule::ParseJsonRule(const Json& json) { } // If the "zip" field is not a regular expression, then it is the sole // postal code for this rule. - if (!ContainsRegExSpecialCharacters(zip)) { - sole_postal_code_ = zip; + if (!ContainsRegExSpecialCharacters(value)) { + sole_postal_code_.swap(value); } } - if (json.HasStringValueForKey(kAdminAreaNameTypeKey)) { + if (json.GetStringValueForKey("state_name_type", &value)) { admin_area_name_message_id_ = - GetMessageIdFromName(json.GetStringValueForKey(kAdminAreaNameTypeKey), - GetAdminAreaMessageIds()); + GetMessageIdFromName(value, GetAdminAreaMessageIds()); } - if (json.HasStringValueForKey(kPostalCodeNameTypeKey)) { + if (json.GetStringValueForKey("zip_name_type", &value)) { postal_code_name_message_id_ = - GetMessageIdFromName(json.GetStringValueForKey(kPostalCodeNameTypeKey), - GetPostalCodeMessageIds()); + GetMessageIdFromName(value, GetPostalCodeMessageIds()); } - if (json.HasStringValueForKey(kNameKey)) { - name_ = json.GetStringValueForKey(kNameKey); + if (json.GetStringValueForKey("name", &value)) { + name_.swap(value); } - if (json.HasStringValueForKey(kLatinNameKey)) { - latin_name_ = json.GetStringValueForKey(kLatinNameKey); + if (json.GetStringValueForKey("lname", &value)) { + latin_name_.swap(value); } - if (json.HasStringValueForKey(kPostalCodeExampleKey)) { - postal_code_example_ = json.GetStringValueForKey(kPostalCodeExampleKey); + if (json.GetStringValueForKey("zipex", &value)) { + postal_code_example_.swap(value); } - if (json.HasStringValueForKey(kPostServiceUrlKey)) { - post_service_url_ = json.GetStringValueForKey(kPostServiceUrlKey); + if (json.GetStringValueForKey("posturl", &value)) { + post_service_url_.swap(value); } } diff --git a/cpp/src/util/json.cc b/cpp/src/util/json.cc index d819bb2..352c62b 100644 --- a/cpp/src/util/json.cc +++ b/cpp/src/util/json.cc @@ -139,23 +139,19 @@ const std::vector& Json::GetKeys() const { return impl_->GetKeys(); } -bool Json::HasStringValueForKey(const std::string& key) const { - assert(impl_ != NULL); - - // Member is owned by impl_. - const Value::Member* member = impl_->FindMember(key); - return member != NULL && member->value.IsString(); -} - -std::string Json::GetStringValueForKey(const std::string& key) const { +bool Json::GetStringValueForKey(const std::string& key, + std::string* value) const { assert(impl_ != NULL); + assert(value != NULL); // Member is owned by impl_. const Value::Member* member = impl_->FindMember(key.c_str()); - assert(member != NULL); - assert(member->value.IsString()); - return std::string(member->value.GetString(), - member->value.GetStringLength()); + if (member == NULL || !member->value.IsString()) { + return false; + } + + value->assign(member->value.GetString(), member->value.GetStringLength()); + return true; } bool Json::HasDictionaryValueForKey(const std::string& key) const { diff --git a/cpp/src/util/json.h b/cpp/src/util/json.h index 1fbf500..90565e6 100644 --- a/cpp/src/util/json.h +++ b/cpp/src/util/json.h @@ -43,15 +43,11 @@ class Json { // successfully in ParseObject() before invoking this method. const std::vector& GetKeys() const; - // Returns true if the parsed JSON contains a string value for |key|. The JSON - // object must be parsed successfully in ParseObject() before invoking this - // method. - bool HasStringValueForKey(const std::string& key) const; - - // Returns the string value for the |key|. The |key| must be present and its - // value must be of string type, i.e., HasStringValueForKey(key) must return - // true before invoking this method. - std::string GetStringValueForKey(const std::string& key) const; + // Returns true if the parsed JSON contains a string value for |key|. Sets + // |value| to the string value of the |key|. The JSON object must be parsed + // successfully in ParseObject() before invoking this method. The |value| + // parameter should not be NULL. + bool GetStringValueForKey(const std::string& key, std::string* value) const; // Returns true if the parsed JSON contains a dictionary value for |key|. The // JSON object must be parsed successfully in ParseObject() before invoking diff --git a/cpp/test/util/json_test.cc b/cpp/test/util/json_test.cc index f6eb389..a3b9f80 100644 --- a/cpp/test/util/json_test.cc +++ b/cpp/test/util/json_test.cc @@ -32,8 +32,9 @@ TEST(JsonTest, EmptyStringIsNotValid) { TEST(JsonTest, EmptyDictionaryContainsNoKeys) { Json json; ASSERT_TRUE(json.ParseObject("{}")); - EXPECT_FALSE(json.HasStringValueForKey("key")); - EXPECT_FALSE(json.HasStringValueForKey(std::string())); + std::string not_checked; + EXPECT_FALSE(json.GetStringValueForKey("key", ¬_checked)); + EXPECT_FALSE(json.GetStringValueForKey(std::string(), ¬_checked)); } TEST(JsonTest, InvalidJsonIsNotValid) { @@ -44,35 +45,40 @@ TEST(JsonTest, InvalidJsonIsNotValid) { TEST(JsonTest, OneKeyIsValid) { Json json; ASSERT_TRUE(json.ParseObject("{\"key\": \"value\"}")); - ASSERT_TRUE(json.HasStringValueForKey("key")); - EXPECT_EQ("value", json.GetStringValueForKey("key")); + std::string value; + EXPECT_TRUE(json.GetStringValueForKey("key", &value)); + EXPECT_EQ("value", value); } TEST(JsonTest, EmptyStringKeyIsNotInObject) { Json json; ASSERT_TRUE(json.ParseObject("{\"key\": \"value\"}")); - EXPECT_FALSE(json.HasStringValueForKey(std::string())); + std::string not_checked; + EXPECT_FALSE(json.GetStringValueForKey(std::string(), ¬_checked)); } TEST(JsonTest, EmptyKeyIsValid) { Json json; ASSERT_TRUE(json.ParseObject("{\"\": \"value\"}")); - ASSERT_TRUE(json.HasStringValueForKey(std::string())); - EXPECT_EQ("value", json.GetStringValueForKey(std::string())); + std::string value; + EXPECT_TRUE(json.GetStringValueForKey(std::string(), &value)); + EXPECT_EQ("value", value); } TEST(JsonTest, EmptyValueIsValid) { Json json; ASSERT_TRUE(json.ParseObject("{\"key\": \"\"}")); - ASSERT_TRUE(json.HasStringValueForKey("key")); - EXPECT_TRUE(json.GetStringValueForKey("key").empty()); + std::string value; + EXPECT_TRUE(json.GetStringValueForKey("key", &value)); + EXPECT_TRUE(value.empty()); } TEST(JsonTest, Utf8EncodingIsValid) { Json json; ASSERT_TRUE(json.ParseObject("{\"key\": \"Ü\"}")); - ASSERT_TRUE(json.HasStringValueForKey("key")); - EXPECT_EQ("Ü", json.GetStringValueForKey("key")); + std::string value; + EXPECT_TRUE(json.GetStringValueForKey("key", &value)); + EXPECT_EQ("Ü", value); } TEST(JsonTest, InvalidUtf8IsNotValid) { @@ -89,11 +95,12 @@ TEST(JsonTest, NullInMiddleIsNotValid) { TEST(JsonTest, TwoKeysAreValid) { Json json; ASSERT_TRUE(json.ParseObject("{\"key1\": \"value1\", \"key2\": \"value2\"}")); - ASSERT_TRUE(json.HasStringValueForKey("key1")); - EXPECT_EQ("value1", json.GetStringValueForKey("key1")); + std::string value; + EXPECT_TRUE(json.GetStringValueForKey("key1", &value)); + EXPECT_EQ("value1", value); - ASSERT_TRUE(json.HasStringValueForKey("key2")); - EXPECT_EQ("value2", json.GetStringValueForKey("key2")); + EXPECT_TRUE(json.GetStringValueForKey("key2", &value)); + EXPECT_EQ("value2", value); } TEST(JsonTest, ListIsNotValid) { @@ -122,8 +129,9 @@ TEST(JsonTest, DictionaryFound) { ASSERT_TRUE(json.ParseObject("{\"key\":{\"inner_key\":\"value\"}}")); ASSERT_TRUE(json.HasDictionaryValueForKey("key")); const Json& sub_json = json.GetDictionaryValueForKey("key"); - ASSERT_TRUE(sub_json.HasStringValueForKey("inner_key")); - EXPECT_EQ("value", sub_json.GetStringValueForKey("inner_key")); + std::string value; + EXPECT_TRUE(sub_json.GetStringValueForKey("inner_key", &value)); + EXPECT_EQ("value", value); } TEST(JsonTest, DictionariesHaveKeys) { -- cgit v1.2.3