From d5ddd14ee1c14ad65bfc82188aac0ca24c412892 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 31 Mar 2015 14:29:32 -0400 Subject: [PATCH] Improve map insertion operations. Add MapInsertOrReplace<>() to insert a key-value pair into a map if the key is not already present, or replace the existing value for key if the key is present. The original value can optionally be returned to the caller in this case. Map insertions now use either MapInsertOrReplace<>() or std::map<>::insert() directly. Use MapInsertOrReplace<>() when the map should be updated to contain a mapping from a key to a value regardless of whether the key is already present. Use std::map<>::insert() to insert a mapping from a key to a value without replacing any existing mapping from a key, if present. If it is important to know whether an existing mapping from a key was present, use the returned std::pair<>.second. If it is important to know the existing value, use the returned std::pair<>.first->second. This change has a slight positive impact on performance. TEST=crashpad_util_test MapInsert.MapInsertOrReplace and others BUG= R=scottmg@chromium.org Review URL: https://codereview.chromium.org/1044273002 --- handler/mac/crash_report_upload_thread.cc | 15 ++--- handler/mac/main.cc | 11 ++-- minidump/minidump_thread_id_map.cc | 3 +- .../mac/mach_o_image_annotations_reader.cc | 7 +-- snapshot/mac/mach_o_image_reader.cc | 9 +-- snapshot/mac/mach_o_image_segment_reader.cc | 12 ++-- .../mac/mach_o_image_symbol_table_reader.cc | 13 +++-- ...inidump_simple_string_dictionary_reader.cc | 6 +- .../minidump/process_snapshot_minidump.cc | 9 +-- tools/mac/run_with_crashpad.cc | 10 ++-- util/net/http_transport_test.cc | 2 +- util/stdlib/map_insert.h | 56 +++++++++++++++++++ util/stdlib/map_insert_test.cc | 54 ++++++++++++++++++ util/util.gyp | 2 + 14 files changed, 154 insertions(+), 55 deletions(-) create mode 100644 util/stdlib/map_insert.h create mode 100644 util/stdlib/map_insert_test.cc diff --git a/handler/mac/crash_report_upload_thread.cc b/handler/mac/crash_report_upload_thread.cc index 5333c78d..e266b7f7 100644 --- a/handler/mac/crash_report_upload_thread.cc +++ b/handler/mac/crash_report_upload_thread.cc @@ -19,7 +19,6 @@ #include #include -#include #include "base/logging.h" #include "base/memory/scoped_ptr.h" @@ -31,6 +30,7 @@ #include "util/net/http_body.h" #include "util/net/http_multipart_builder.h" #include "util/net/http_transport.h" +#include "util/stdlib/map_insert.h" namespace crashpad { @@ -39,13 +39,10 @@ namespace { void InsertOrReplaceMapEntry(std::map* map, const std::string& key, const std::string& value) { - auto it = map->find(key); - if (it != map->end()) { + std::string old_value; + if (!MapInsertOrReplace(map, key, value, &old_value)) { LOG(WARNING) << "duplicate key " << key << ", discarding value " - << it->second; - it->second = value; - } else { - map->insert(std::make_pair(key, value)); + << old_value; } } @@ -75,11 +72,9 @@ std::map BreakpadHTTPFormParametersFromMinidump( std::string list_annotations; for (const ModuleSnapshot* module : minidump_process_snapshot.Modules()) { for (const auto& kv : module->AnnotationsSimpleMap()) { - if (parameters.find(kv.first) != parameters.end()) { + if (!parameters.insert(kv).second) { LOG(WARNING) << "duplicate key " << kv.first << ", discarding value " << kv.second; - } else { - parameters.insert(kv); } } diff --git a/handler/mac/main.cc b/handler/mac/main.cc index 1df183f9..7f0629bb 100644 --- a/handler/mac/main.cc +++ b/handler/mac/main.cc @@ -18,7 +18,6 @@ #include #include -#include #include "base/files/file_path.h" #include "base/logging.h" @@ -30,6 +29,7 @@ #include "handler/mac/exception_handler_server.h" #include "util/mach/child_port_handshake.h" #include "util/posix/close_stdio.h" +#include "util/stdlib/map_insert.h" #include "util/stdlib/string_number_conversion.h" #include "util/string/split_string.h" #include "util/synchronization/semaphore.h" @@ -97,13 +97,10 @@ int HandlerMain(int argc, char* argv[]) { ToolSupport::UsageHint(me, "--annotation requires KEY=VALUE"); return EXIT_FAILURE; } - auto it = options.annotations.find(key); - if (it != options.annotations.end()) { + std::string old_value; + if (!MapInsertOrReplace(&options.annotations, key, value, &old_value)) { LOG(WARNING) << "duplicate key " << key << ", discarding value " - << it->second; - it->second = value; - } else { - options.annotations.insert(std::make_pair(key, value)); + << old_value; } break; } diff --git a/minidump/minidump_thread_id_map.cc b/minidump/minidump_thread_id_map.cc index 1fab698e..327d12e1 100644 --- a/minidump/minidump_thread_id_map.cc +++ b/minidump/minidump_thread_id_map.cc @@ -38,11 +38,10 @@ void BuildMinidumpThreadIDMap( uint64_t thread_id_64 = thread_snapshot->ThreadID(); if (thread_id_map->find(thread_id_64) == thread_id_map->end()) { uint32_t thread_id_32 = static_cast(thread_id_64); - if (thread_ids_32.find(thread_id_32) != thread_ids_32.end()) { + if (!thread_ids_32.insert(thread_id_32).second) { collision = true; break; } - thread_ids_32.insert(thread_id_32); thread_id_map->insert(std::make_pair(thread_id_64, thread_id_32)); } } diff --git a/snapshot/mac/mach_o_image_annotations_reader.cc b/snapshot/mac/mach_o_image_annotations_reader.cc index 4623aa63..6c6831bd 100644 --- a/snapshot/mac/mach_o_image_annotations_reader.cc +++ b/snapshot/mac/mach_o_image_annotations_reader.cc @@ -17,6 +17,8 @@ #include #include +#include + #include "base/logging.h" #include "client/crashpad_info.h" #include "client/simple_string_dictionary.h" @@ -161,10 +163,7 @@ void MachOImageAnnotationsReader::ReadCrashpadSimpleAnnotations( if (key_length) { std::string key(entry.key, key_length); std::string value(entry.value, strnlen(entry.value, sizeof(entry.value))); - if (!simple_map_annotations->count(key)) { - simple_map_annotations->insert( - std::pair(key, value)); - } else { + if (!simple_map_annotations->insert(std::make_pair(key, value)).second) { LOG(INFO) << "duplicate simple annotation " << key << " in " << name_; } } diff --git a/snapshot/mac/mach_o_image_reader.cc b/snapshot/mac/mach_o_image_reader.cc index f9da354d..cb85baab 100644 --- a/snapshot/mac/mach_o_image_reader.cc +++ b/snapshot/mac/mach_o_image_reader.cc @@ -20,6 +20,7 @@ #include #include +#include #include "base/logging.h" #include "base/strings/stringprintf.h" @@ -543,15 +544,15 @@ bool MachOImageReader::ReadSegmentCommand( // become inconsistent or require cleanup. const std::string segment_name = segment->Name(); - const auto& iterator = segment_map_.find(segment_name); - if (iterator != segment_map_.end()) { + const auto insert_result = + segment_map_.insert(std::make_pair(segment_name, segment_index)); + if (!insert_result.second) { LOG(WARNING) << base::StringPrintf("duplicate %s segment at %zu and %zu", segment_name.c_str(), - iterator->second, + insert_result.first->second, segment_index) << load_command_info; return false; } - segment_map_[segment_name] = segment_index; mach_vm_size_t vmsize = segment->vmsize(); diff --git a/snapshot/mac/mach_o_image_segment_reader.cc b/snapshot/mac/mach_o_image_segment_reader.cc index 6adcd6f2..8a077dde 100644 --- a/snapshot/mac/mach_o_image_segment_reader.cc +++ b/snapshot/mac/mach_o_image_segment_reader.cc @@ -16,6 +16,8 @@ #include +#include + #include "base/logging.h" #include "base/strings/stringprintf.h" #include "snapshot/mac/process_reader.h" @@ -185,14 +187,14 @@ bool MachOImageSegmentReader::Initialize(ProcessReader* process_reader, return false; } - const auto& iterator = section_map_.find(section_name); - if (iterator != section_map_.end()) { + const auto insert_result = + section_map_.insert(std::make_pair(section_name, section_index)); + if (!insert_result.second) { LOG(WARNING) << base::StringPrintf("duplicate section name at %zu", - iterator->second) << section_info; + insert_result.first->second) + << section_info; return false; } - - section_map_[section_name] = section_index; } INITIALIZATION_STATE_SET_VALID(initialized_); diff --git a/snapshot/mac/mach_o_image_symbol_table_reader.cc b/snapshot/mac/mach_o_image_symbol_table_reader.cc index 12d8de7e..949e4608 100644 --- a/snapshot/mac/mach_o_image_symbol_table_reader.cc +++ b/snapshot/mac/mach_o_image_symbol_table_reader.cc @@ -17,6 +17,8 @@ #include #include +#include + #include "base/memory/scoped_ptr.h" #include "base/strings/stringprintf.h" #include "util/mac/checked_mach_address_range.h" @@ -153,15 +155,14 @@ class MachOImageSymbolTableReaderInitializer { return false; } - if (external_defined_symbols->count(name)) { - LOG(WARNING) << "duplicate symbol " << name << symbol_info; - return false; - } - MachOImageSymbolTableReader::SymbolInformation this_symbol_info; this_symbol_info.value = symbol.n_value; this_symbol_info.section = symbol.n_sect; - (*external_defined_symbols)[name] = this_symbol_info; + if (!external_defined_symbols->insert( + std::make_pair(name, this_symbol_info)).second) { + LOG(WARNING) << "duplicate symbol " << name << symbol_info; + return false; + } } else { // External indirect symbols may be found in the portion of the symbol // table used for external symbols as opposed to indirect symbols when diff --git a/snapshot/minidump/minidump_simple_string_dictionary_reader.cc b/snapshot/minidump/minidump_simple_string_dictionary_reader.cc index 085c1f60..0d8301c1 100644 --- a/snapshot/minidump/minidump_simple_string_dictionary_reader.cc +++ b/snapshot/minidump/minidump_simple_string_dictionary_reader.cc @@ -72,12 +72,10 @@ bool ReadMinidumpSimpleStringDictionary( return false; } - if (local_dictionary.find(key) != local_dictionary.end()) { - LOG(WARNING) << "duplicate key " << key << ", discarding value " << value; + if (!local_dictionary.insert(std::make_pair(key, value)).second) { + LOG(ERROR) << "duplicate key " << key; return false; } - - local_dictionary.insert(std::make_pair(key, value)); } dictionary->swap(local_dictionary); diff --git a/snapshot/minidump/process_snapshot_minidump.cc b/snapshot/minidump/process_snapshot_minidump.cc index 256e429e..fa87981b 100644 --- a/snapshot/minidump/process_snapshot_minidump.cc +++ b/snapshot/minidump/process_snapshot_minidump.cc @@ -309,16 +309,13 @@ bool ProcessSnapshotMinidump::InitializeModulesCrashpadInfo( ++crashpad_module_index) { const MinidumpModuleCrashpadInfoLink& minidump_link = minidump_links[crashpad_module_index]; - if (module_crashpad_info_links->find( - minidump_link.minidump_module_list_index) != - module_crashpad_info_links->end()) { + if (!module_crashpad_info_links + ->insert(std::make_pair(minidump_link.minidump_module_list_index, + minidump_link.location)).second) { LOG(WARNING) << "duplicate module_crashpad_info_list minidump_module_list_index " << minidump_link.minidump_module_list_index; return false; - } else { - module_crashpad_info_links->insert(std::make_pair( - minidump_link.minidump_module_list_index, minidump_link.location)); } } diff --git a/tools/mac/run_with_crashpad.cc b/tools/mac/run_with_crashpad.cc index 9d6679cf..0b4653aa 100644 --- a/tools/mac/run_with_crashpad.cc +++ b/tools/mac/run_with_crashpad.cc @@ -27,6 +27,7 @@ #include "base/logging.h" #include "client/crashpad_client.h" #include "tools/tool_support.h" +#include "util/stdlib/map_insert.h" #include "util/string/split_string.h" namespace crashpad { @@ -118,13 +119,10 @@ int RunWithCrashpadMain(int argc, char* argv[]) { ToolSupport::UsageHint(me, "--annotation requires KEY=VALUE"); return EXIT_FAILURE; } - auto it = options.annotations.find(key); - if (it != options.annotations.end()) { + std::string old_value; + if (!MapInsertOrReplace(&options.annotations, key, value, &old_value)) { LOG(WARNING) << "duplicate key " << key << ", discarding value " - << it->second; - it->second = value; - } else { - options.annotations.insert(std::make_pair(key, value)); + << old_value; } break; } diff --git a/util/net/http_transport_test.cc b/util/net/http_transport_test.cc index ddda100e..375ccfc3 100644 --- a/util/net/http_transport_test.cc +++ b/util/net/http_transport_test.cc @@ -222,7 +222,7 @@ TEST(HTTPTransport, ValidFormData) { builder.SetFormData("key2", "--abcdefg123"); HTTPHeaders headers; - headers.insert(builder.GetContentType()); + EXPECT_TRUE(headers.insert(builder.GetContentType()).second); HTTPTransportTestFixture test(headers, builder.GetBodyStream(), 200, &ValidFormData); diff --git a/util/stdlib/map_insert.h b/util/stdlib/map_insert.h new file mode 100644 index 00000000..956271b3 --- /dev/null +++ b/util/stdlib/map_insert.h @@ -0,0 +1,56 @@ +// Copyright 2015 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef CRASHPAD_UTIL_STDLIB_MAP_INSERT_H_ +#define CRASHPAD_UTIL_STDLIB_MAP_INSERT_H_ + +#include +#include + +namespace crashpad { + +//! \brief Inserts a mapping from \a key to \a value into \a map, or replaces +//! an existing mapping so that \a key maps to \value. +//! +//! This behaves similarly to `std::map<>::insert_or_assign()` proposed for +//! C++17, except that the \a old_value parameter is added. +//! +//! \param[inout] map The map to operate on. +//! \param[in] key The key that should be mapped to \a value. +//! \param[in] value The value that \a key should map to. +//! \param[out] old_value If \a key was previously present in \a map, this will +//! be set to its previous value. This parameter is optional and may be +//! `nullptr` if this information is not required. +//! +//! \return `false` if \a key was previously present in \a map. If \a old_value +//! is not `nullptr`, it will be set to the previous value. `true` if \a +//! key was not present in the map and was inserted. +template +bool MapInsertOrReplace(T* map, + const typename T::key_type& key, + const typename T::mapped_type& value, + typename T::mapped_type* old_value) { + const auto result = map->insert(std::make_pair(key, value)); + if (!result.second) { + if (old_value) { + *old_value = result.first->second; + } + result.first->second = value; + } + return result.second; +} + +} // namespace crashpad + +#endif // CRASHPAD_UTIL_STDLIB_MAP_INSERT_H_ diff --git a/util/stdlib/map_insert_test.cc b/util/stdlib/map_insert_test.cc new file mode 100644 index 00000000..d3197b96 --- /dev/null +++ b/util/stdlib/map_insert_test.cc @@ -0,0 +1,54 @@ +// Copyright 2015 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "util/stdlib/map_insert.h" + +#include + +#include "gtest/gtest.h" + +namespace crashpad { +namespace test { +namespace { + +TEST(MapInsert, MapInsertOrReplace) { + std::map map; + int old_value; + EXPECT_TRUE(MapInsertOrReplace(&map, "key", 1, &old_value)); + std::map expect_map; + expect_map["key"] = 1; + EXPECT_EQ(expect_map, map); + + EXPECT_FALSE(MapInsertOrReplace(&map, "key", 2, &old_value)); + EXPECT_EQ(1, old_value); + expect_map["key"] = 2; + EXPECT_EQ(expect_map, map); + + EXPECT_TRUE(MapInsertOrReplace(&map, "another", 3, &old_value)); + expect_map["another"] = 3; + EXPECT_EQ(expect_map, map); + + // Make sure nullptr is accepted as old_value. + EXPECT_TRUE(MapInsertOrReplace(&map, "yet another", 5, nullptr)); + expect_map["yet another"] = 5; + EXPECT_EQ(expect_map, map); + + EXPECT_FALSE(MapInsertOrReplace(&map, "yet another", 6, nullptr)); + expect_map["yet another"] = 6; + EXPECT_EQ(expect_map, map); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/util/util.gyp b/util/util.gyp index e94377cf..404d69c8 100644 --- a/util/util.gyp +++ b/util/util.gyp @@ -121,6 +121,7 @@ 'posix/symbolic_constants_posix.cc', 'posix/symbolic_constants_posix.h', 'stdlib/cxx.h', + 'stdlib/map_insert.h', 'stdlib/objc.h', 'stdlib/pointer_container.h', 'stdlib/string_number_conversion.cc', @@ -319,6 +320,7 @@ 'numeric/int128_test.cc', 'posix/process_info_test.cc', 'posix/symbolic_constants_posix_test.cc', + 'stdlib/map_insert_test.cc', 'stdlib/string_number_conversion_test.cc', 'stdlib/strlcpy_test.cc', 'stdlib/strnlen_test.cc',