Use ad hoc itoa function instead of OS::SNPrintF when serializing heap snapshots

src/profile-generator.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 3335 matching lines @@
     while (s < s_end) {
       int s_chunk_size = Min(
           chunk_size_ - chunk_pos_, static_cast<int>(s_end - s));
       ASSERT(s_chunk_size > 0);
       memcpy(chunk_.start() + chunk_pos_, s, s_chunk_size);
       s += s_chunk_size;
       chunk_pos_ += s_chunk_size;
       MaybeWriteChunk();
     }
   }
-  void AddNumber(int n) { AddNumberImpl<int>(n, "%d"); }
   void AddNumber(unsigned n) { AddNumberImpl<unsigned>(n, "%u"); }
-  void AddNumber(uint64_t n) { AddNumberImpl<uint64_t>(n, "%llu"); }
   void Finalize() {
     if (aborted_) return;
     ASSERT(chunk_pos_ < chunk_size_);
     if (chunk_pos_ != 0) {
       WriteChunk();
     }
     stream_->EndOfStream();
   }
 
  private:
@@ skipping 134 matching lines @@
 int HeapSnapshotJSONSerializer::GetStringId(const char* s) {
   HashMap::Entry* cache_entry = strings_.Lookup(
       const_cast<char*>(s), ObjectHash(s), true);
   if (cache_entry->value == NULL) {
     cache_entry->value = reinterpret_cast<void*>(next_string_id_++);
   }
   return static_cast<int>(reinterpret_cast<intptr_t>(cache_entry->value));
 }
 
 
+static char* itoa(int value, char* buffer) {

[mnaganov (inactive), 2012/04/04 10:03:31]

Have you tried IntToCString? It uses Vector, which has overrun asserts in debug
mode.

[alexeif, 2012/04/04 11:20:49]

I have more concerns regarding the checks in release mode.

[yurys, 2012/04/04 12:32:16]

IntToCString will place the number into the end of the buffer which wouldn't
work for us as we don't know number of digits beforehand. I switched this
implementation to Vector<char> so that we have overrun asserts in debug mode.

+  if (value < 0) {
+    *buffer = '-';
+    ++buffer;
+    value = -value;
+  }
+
+  int number_of_digits = 0;
+  int t = value;
+  do {
+    ++number_of_digits;
+  } while (t /= 10);
+
+  buffer += number_of_digits;
+  char* result = buffer;
+  do {
+    int last_digit = value % 10;
+    value /= 10;
+    *--buffer = '0' + last_digit;
+  } while (value);
+  return result;
+}
+
+
 void HeapSnapshotJSONSerializer::SerializeEdge(HeapGraphEdge* edge) {
   // The buffer needs space for 3 ints, 3 commas and \0
   static const int kBufferSize =
       MaxDecimalDigitsIn<sizeof(int)>::kSigned * 3 + 3 + 1;  // NOLINT
   EmbeddedVector<char, kBufferSize> buffer;
   int edge_name_or_index = edge->type() == HeapGraphEdge::kElement
       || edge->type() == HeapGraphEdge::kHidden
       || edge->type() == HeapGraphEdge::kWeak
       ? edge->index() : GetStringId(edge->name());
   STATIC_CHECK(sizeof(int) == sizeof(edge->type()));  // NOLINT
   STATIC_CHECK(sizeof(int) == sizeof(edge_name_or_index));  // NOLINT
   STATIC_CHECK(sizeof(int) == sizeof(GetNodeId(edge->to())));  // NOLINT
-  int result = OS::SNPrintF(buffer, ",%d,%d,%d",
-      edge->type(), edge_name_or_index, GetNodeId(edge->to()));
-  USE(result);
-  ASSERT(result != -1);
+  char* buffer_pos = buffer.start();
+  *buffer_pos++ = ',';
+  buffer_pos = itoa(edge->type(), buffer_pos);
+  *buffer_pos++ = ',';
+  buffer_pos = itoa(edge_name_or_index, buffer_pos);
+  *buffer_pos++ = ',';
+  buffer_pos = itoa(GetNodeId(edge->to()), buffer_pos);
+  *buffer_pos++ = '\0';
+  ASSERT(buffer_pos <= (buffer.start() + buffer.length()));
   writer_->AddString(buffer.start());
 }
 
 
 void HeapSnapshotJSONSerializer::SerializeNode(HeapEntry* entry) {
   // The buffer needs space for 6 ints, 1 uint32_t, 7 commas, \n and \0
   static const int kBufferSize =
       6 * MaxDecimalDigitsIn<sizeof(int)>::kSigned  // NOLINT
       + MaxDecimalDigitsIn<sizeof(uint32_t)>::kUnsigned  // NOLINT
       + 7 + 1 + 1;
   EmbeddedVector<char, kBufferSize> buffer;
   Vector<HeapGraphEdge> children = entry->children();
   STATIC_CHECK(sizeof(int) == sizeof(entry->type()));  // NOLINT

[alexeif, 2012/04/04 11:20:49]

If you remove printf these checks are unnecessary.

[yurys, 2012/04/04 12:32:16]

Done. Removed.

   STATIC_CHECK(sizeof(int) == sizeof(GetStringId(entry->name())));  // NOLINT
   STATIC_CHECK(sizeof(unsigned) == sizeof(entry->id()));  // NOLINT
   STATIC_CHECK(sizeof(int) == sizeof(entry->self_size()));  // NOLINT
   STATIC_CHECK(sizeof(int) == sizeof(entry->retained_size()));  // NOLINT
   STATIC_CHECK(sizeof(int) == sizeof(GetNodeId(entry->dominator())));  // NOLINT
   STATIC_CHECK(sizeof(int) == sizeof(children.length()));  // NOLINT
-  int result = OS::SNPrintF(buffer, "\n,%d,%d,%u,%d,%d,%d,%d",
-      entry->type(),
-      GetStringId(entry->name()),
-      entry->id(),
-      entry->self_size(),
-      entry->retained_size(),
-      GetNodeId(entry->dominator()),
-      children.length());
-  USE(result);
-  ASSERT(result != -1);
+  char* buffer_pos = buffer.start();
+  *buffer_pos++ = '\n';
+  *buffer_pos++ = ',';
+  buffer_pos = itoa(entry->type(), buffer_pos);
+  *buffer_pos++ = ',';
+  buffer_pos = itoa(GetStringId(entry->name()), buffer_pos);
+  *buffer_pos++ = ',';
+  buffer_pos = itoa(entry->id(), buffer_pos);
+  *buffer_pos++ = ',';
+  buffer_pos = itoa(entry->self_size(), buffer_pos);
+  *buffer_pos++ = ',';
+  buffer_pos = itoa(entry->retained_size(), buffer_pos);
+  *buffer_pos++ = ',';
+  buffer_pos = itoa(GetNodeId(entry->dominator()), buffer_pos);
+  *buffer_pos++ = ',';
+  buffer_pos = itoa(children.length(), buffer_pos);
+  *buffer_pos++ = '\0';
+  ASSERT(buffer_pos <= (buffer.start() + buffer.length()));
   writer_->AddString(buffer.start());
   for (int i = 0; i < children.length(); ++i) {
     SerializeEdge(&children[i]);
     if (writer_->aborted()) return;
   }
 }
 
 
 void HeapSnapshotJSONSerializer::SerializeNodes() {
   // The first (zero) item of nodes array is an object describing node
@@ skipping 171 matching lines @@
 
 
 void HeapSnapshotJSONSerializer::SortHashMap(
     HashMap* map, List<HashMap::Entry*>* sorted_entries) {
   for (HashMap::Entry* p = map->Start(); p != NULL; p = map->Next(p))
     sorted_entries->Add(p);
   sorted_entries->Sort(SortUsingEntryValue);
 }
 
 } }  // namespace v8::internal