Pool TypeName strings when writing MINIDUMP_HANDLE_DESCRIPTOR

minidump/minidump_handle_writer.cc

Index: minidump/minidump_handle_writer.cc
diff --git a/minidump/minidump_handle_writer.cc b/minidump/minidump_handle_writer.cc
index 235d71b0db995ffec7db6e1f3314015084f4ad22..ff9df420400afb5f29a137d7ed48e75336c3ce59 100644
--- a/minidump/minidump_handle_writer.cc
+++ b/minidump/minidump_handle_writer.cc
@@ -17,6 +17,7 @@
 #include <string>
 
 #include "base/logging.h"
+#include "base/stl_util.h"
 #include "minidump/minidump_extensions.h"
 #include "util/file/file_writer.h"
 
@@ -27,6 +28,7 @@ MinidumpHandleDataWriter::MinidumpHandleDataWriter()
 }
 
 MinidumpHandleDataWriter::~MinidumpHandleDataWriter() {
+  STLDeleteContainerPairSecondPointers(strings_.begin(), strings_.end());
 }
 
 void MinidumpHandleDataWriter::InitializeFromSnapshot(
@@ -37,7 +39,6 @@ void MinidumpHandleDataWriter::InitializeFromSnapshot(
   // Because we RegisterRVA() on the string writer below, we preallocate and
   // never resize the handle_descriptors_ vector.
   handle_descriptors_.resize(handle_snapshots.size());
-  strings_.reserve(handle_snapshots.size());
   for (size_t i = 0; i < handle_snapshots.size(); ++i) {
     const HandleSnapshot& handle_snapshot = handle_snapshots[i];
     MINIDUMP_HANDLE_DESCRIPTOR& descriptor = handle_descriptors_[i];
@@ -47,11 +48,16 @@ void MinidumpHandleDataWriter::InitializeFromSnapshot(
     if (handle_snapshot.type_name.empty()) {
       descriptor.TypeNameRva = 0;
     } else {
-      // TODO(scottmg): There is often a number of repeated type names here, the
-      // strings ought to be pooled.
-      strings_.push_back(new internal::MinidumpUTF16StringWriter());
-      strings_.back()->SetUTF16(handle_snapshot.type_name);
-      strings_.back()->RegisterRVA(&descriptor.TypeNameRva);
+      auto it = strings_.find(handle_snapshot.type_name);

[Mark Mentovai, 2015/10/21 18:27:01]

std::map<> has a really annoying interface for this. It kind of sucks that
find() does a lookup here and then the operator[] potentially does the identical
lookup below.

Instead of find(), you could lower_bound(). Then the conditional below becomes
|it != end() && it->first == type_name|, and in the else branch, you could
insert(it, value_type(type_name, writer)).

The other alternative is to insert(value_type(type_name, writer)) up front
without first checking to see whether it’s present. If the key was already
there, the returned pair.second will be false, and pair.first will be the
existing writer. That’s nominally cleaner than the
lower_bound()+double-test+insert() approach, but since writer’s a pointer to a
newly-allocated thing, you’re still on the hook for deleting it.

Does it matter much here? Probably not. But the alternatives are probably just
as little code as this, maybe even less.

[scottmg, 2015/10/21 18:46:10]

Done.

+      internal::MinidumpUTF16StringWriter* writer;
+      if (it != strings_.end()) {
+        writer = it->second;
+      } else {
+        writer = new internal::MinidumpUTF16StringWriter();
+        strings_[handle_snapshot.type_name] = writer;
+        writer->SetUTF16(handle_snapshot.type_name);
+      }
+      writer->RegisterRVA(&descriptor.TypeNameRva);
     }
 
     descriptor.ObjectNameRva = 0;
@@ -86,8 +92,8 @@ std::vector<internal::MinidumpWritable*> MinidumpHandleDataWriter::Children() {
   DCHECK_GE(state(), kStateFrozen);
 
   std::vector<MinidumpWritable*> children;
-  for (auto* string : strings_)
-    children.push_back(string);
+  for (const auto& pair : strings_)
+    children.push_back(pair.second);
   return children;
 }