Bug 1635570 - Cut too-long label dynamic strings and add ellipsis - r=canaltinova
authorGerald Squelart <gsquelart@mozilla.com>
Tue, 12 May 2020 01:13:17 +0000
changeset 529273 9923799dd260cb125fb149b92298034ca3beb112
parent 529272 3a50d857d0979b25bf7f9fe24234bc6b3d9fb464
child 529274 040a6d3c401de4142568a0c942f169244dc72a71
push id37406
push userdluca@mozilla.com
push dateTue, 12 May 2020 09:34:21 +0000
treeherdermozilla-central@1706d4d54ec6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscanaltinova
bugs1635570
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1635570 - Cut too-long label dynamic strings and add ellipsis - r=canaltinova If a label contains a dynamic string that's too long (512 characters or more), instead of just replacing it with "(too long)", we now cut it down to the maximum size, with an ellipsis at the end. Added test for that in gtest. Also added nearby test for empty strings. Differential Revision: https://phabricator.services.mozilla.com/D74378
mozglue/baseprofiler/core/ProfileBuffer.cpp
tools/profiler/core/ProfileBuffer.cpp
tools/profiler/tests/gtest/GeckoProfiler.cpp
--- a/mozglue/baseprofiler/core/ProfileBuffer.cpp
+++ b/mozglue/baseprofiler/core/ProfileBuffer.cpp
@@ -68,27 +68,47 @@ void ProfileBuffer::CollectCodeLocation(
     const Maybe<uint32_t>& aColumnNumber,
     const Maybe<ProfilingCategoryPair>& aCategoryPair) {
   AddEntry(ProfileBufferEntry::Label(aLabel));
   AddEntry(ProfileBufferEntry::FrameFlags(uint64_t(aFrameFlags)));
 
   if (aStr) {
     // Store the string using one or more DynamicStringFragment entries.
     size_t strLen = strlen(aStr) + 1;  // +1 for the null terminator
-    for (size_t j = 0; j < strLen;) {
+    // If larger than the prescribed limit, we will cut the string and end it
+    // with an ellipsis.
+    const bool tooBig = strLen > kMaxFrameKeyLength;
+    if (tooBig) {
+      strLen = kMaxFrameKeyLength;
+    }
+    char chars[ProfileBufferEntry::kNumChars];
+    for (size_t j = 0;; j += ProfileBufferEntry::kNumChars) {
       // Store up to kNumChars characters in the entry.
-      char chars[ProfileBufferEntry::kNumChars];
       size_t len = ProfileBufferEntry::kNumChars;
-      if (j + len >= strLen) {
+      const bool last = j + len >= strLen;
+      if (last) {
+        // Only the last entry may be smaller than kNumChars.
         len = strLen - j;
+        if (tooBig) {
+          // That last entry is part of a too-big string, replace the end
+          // characters with an ellipsis "...".
+          len = std::max(len, size_t(4));
+          chars[len - 4] = '.';
+          chars[len - 3] = '.';
+          chars[len - 2] = '.';
+          chars[len - 1] = '\0';
+          // Make sure the memcpy will not overwrite our ellipsis!
+          len -= 4;
+        }
       }
       memcpy(chars, &aStr[j], len);
-      j += ProfileBufferEntry::kNumChars;
-
       AddEntry(ProfileBufferEntry::DynamicStringFragment(chars));
+      if (last) {
+        break;
+      }
     }
   }
 
   if (aInnerWindowID) {
     AddEntry(ProfileBufferEntry::InnerWindowID(aInnerWindowID));
   }
 
   if (aLineNumber) {
@@ -188,18 +208,16 @@ void ProfileBufferCollector::CollectProf
   Maybe<uint32_t> column;
 
   MOZ_ASSERT(aFrame.isLabelFrame());
 
   if (dynamicString) {
     // Adjust the dynamic string as necessary.
     if (ProfilerFeature::HasPrivacy(mFeatures) && !isChromeJSEntry) {
       dynamicString = "(private)";
-    } else if (strlen(dynamicString) >= ProfileBuffer::kMaxFrameKeyLength) {
-      dynamicString = "(too long)";
     }
   }
 
   mBuf.CollectCodeLocation(label, dynamicString, aFrame.flags(),
                            aFrame.realmID(), line, column,
                            Some(aFrame.categoryPair()));
 }
 
--- a/tools/profiler/core/ProfileBuffer.cpp
+++ b/tools/profiler/core/ProfileBuffer.cpp
@@ -69,27 +69,47 @@ void ProfileBuffer::CollectCodeLocation(
     const Maybe<uint32_t>& aColumnNumber,
     const Maybe<JS::ProfilingCategoryPair>& aCategoryPair) {
   AddEntry(ProfileBufferEntry::Label(aLabel));
   AddEntry(ProfileBufferEntry::FrameFlags(uint64_t(aFrameFlags)));
 
   if (aStr) {
     // Store the string using one or more DynamicStringFragment entries.
     size_t strLen = strlen(aStr) + 1;  // +1 for the null terminator
-    for (size_t j = 0; j < strLen;) {
+    // If larger than the prescribed limit, we will cut the string and end it
+    // with an ellipsis.
+    const bool tooBig = strLen > kMaxFrameKeyLength;
+    if (tooBig) {
+      strLen = kMaxFrameKeyLength;
+    }
+    char chars[ProfileBufferEntry::kNumChars];
+    for (size_t j = 0;; j += ProfileBufferEntry::kNumChars) {
       // Store up to kNumChars characters in the entry.
-      char chars[ProfileBufferEntry::kNumChars];
       size_t len = ProfileBufferEntry::kNumChars;
-      if (j + len >= strLen) {
+      const bool last = j + len >= strLen;
+      if (last) {
+        // Only the last entry may be smaller than kNumChars.
         len = strLen - j;
+        if (tooBig) {
+          // That last entry is part of a too-big string, replace the end
+          // characters with an ellipsis "...".
+          len = std::max(len, size_t(4));
+          chars[len - 4] = '.';
+          chars[len - 3] = '.';
+          chars[len - 2] = '.';
+          chars[len - 1] = '\0';
+          // Make sure the memcpy will not overwrite our ellipsis!
+          len -= 4;
+        }
       }
       memcpy(chars, &aStr[j], len);
-      j += ProfileBufferEntry::kNumChars;
-
       AddEntry(ProfileBufferEntry::DynamicStringFragment(chars));
+      if (last) {
+        break;
+      }
     }
   }
 
   if (aInnerWindowID) {
     AddEntry(ProfileBufferEntry::InnerWindowID(aInnerWindowID));
   }
 
   if (aLineNumber) {
@@ -224,17 +244,15 @@ void ProfileBufferCollector::CollectProf
   } else {
     MOZ_ASSERT(aFrame.isLabelFrame());
   }
 
   if (dynamicString) {
     // Adjust the dynamic string as necessary.
     if (ProfilerFeature::HasPrivacy(mFeatures) && !isChromeJSEntry) {
       dynamicString = "(private)";
-    } else if (strlen(dynamicString) >= ProfileBuffer::kMaxFrameKeyLength) {
-      dynamicString = "(too long)";
     }
   }
 
   mBuf.CollectCodeLocation(label, dynamicString, aFrame.flags(),
                            aFrame.realmID(), line, column,
                            Some(aFrame.categoryPair()));
 }
--- a/tools/profiler/tests/gtest/GeckoProfiler.cpp
+++ b/tools/profiler/tests/gtest/GeckoProfiler.cpp
@@ -625,30 +625,42 @@ TEST(GeckoProfiler, Markers)
   EXPECT_EQ(GTestMarkerPayload::sNumDestroyed, 10);
 
   // Create three strings: two that are the maximum allowed length, and one that
   // is one char longer.
   static const size_t kMax = ProfileBuffer::kMaxFrameKeyLength;
   UniquePtr<char[]> okstr1 = MakeUnique<char[]>(kMax);
   UniquePtr<char[]> okstr2 = MakeUnique<char[]>(kMax);
   UniquePtr<char[]> longstr = MakeUnique<char[]>(kMax + 1);
+  UniquePtr<char[]> longstrCut = MakeUnique<char[]>(kMax + 1);
   for (size_t i = 0; i < kMax; i++) {
     okstr1[i] = 'a';
     okstr2[i] = 'b';
     longstr[i] = 'c';
+    longstrCut[i] = 'c';
   }
   okstr1[kMax - 1] = '\0';
   okstr2[kMax - 1] = '\0';
   longstr[kMax] = '\0';
+  longstrCut[kMax] = '\0';
   // Should be output as-is.
+  AUTO_PROFILER_LABEL_DYNAMIC_CSTR("", LAYOUT, "");
   AUTO_PROFILER_LABEL_DYNAMIC_CSTR("", LAYOUT, okstr1.get());
   // Should be output as label + space + okstr2.
   AUTO_PROFILER_LABEL_DYNAMIC_CSTR("okstr2", LAYOUT, okstr2.get());
-  // Should be output as "(too long)".
+  // Should be output with kMax length, ending with "...\0".
   AUTO_PROFILER_LABEL_DYNAMIC_CSTR("", LAYOUT, longstr.get());
+  ASSERT_EQ(longstrCut[kMax - 4], 'c');
+  longstrCut[kMax - 4] = '.';
+  ASSERT_EQ(longstrCut[kMax - 3], 'c');
+  longstrCut[kMax - 3] = '.';
+  ASSERT_EQ(longstrCut[kMax - 2], 'c');
+  longstrCut[kMax - 2] = '.';
+  ASSERT_EQ(longstrCut[kMax - 1], 'c');
+  longstrCut[kMax - 1] = '\0';
 
   // Used in markers below.
   TimeStamp ts1 = TimeStamp::NowUnfuzzed();
 
   // Sleep briefly to ensure a sample is taken and the pending markers are
   // processed.
   PR_Sleep(PR_MillisecondsToInterval(500));
 
@@ -892,36 +904,41 @@ TEST(GeckoProfiler, Markers)
       // root.threads[0] is an object.
 
       // Keep a reference to the string table in this block, it will be used
       // below.
       const Json::Value& stringTable = thread0["stringTable"];
       ASSERT_TRUE(stringTable.isArray());
 
       // Test the expected labels in the string table.
+      bool foundEmpty = false;
       bool foundOkstr1 = false;
       bool foundOkstr2 = false;
       const std::string okstr2Label = std::string("okstr2 ") + okstr2.get();
       bool foundTooLong = false;
       for (const auto& s : stringTable) {
         ASSERT_TRUE(s.isString());
         std::string sString = s.asString();
-        if (sString == okstr1.get()) {
+        if (sString.empty()) {
+          EXPECT_FALSE(foundEmpty);
+          foundEmpty = true;
+        } else if (sString == okstr1.get()) {
           EXPECT_FALSE(foundOkstr1);
           foundOkstr1 = true;
         } else if (sString == okstr2Label) {
           EXPECT_FALSE(foundOkstr2);
           foundOkstr2 = true;
-        } else if (sString == "(too long)") {
+        } else if (sString == longstrCut.get()) {
           EXPECT_FALSE(foundTooLong);
           foundTooLong = true;
         } else {
           EXPECT_NE(sString, longstr.get());
         }
       }
+      EXPECT_TRUE(foundEmpty);
       EXPECT_TRUE(foundOkstr1);
       EXPECT_TRUE(foundOkstr2);
       EXPECT_TRUE(foundTooLong);
 
       {
         const Json::Value& markers = thread0["markers"];
         ASSERT_TRUE(markers.isObject());