bug 1505060 - Support multistore for dynamic scalars r=janerik
authorChris H-C <chutten@mozilla.com>
Tue, 27 Nov 2018 17:48:46 +0000
changeset 504803 e54386a858606c13bd4c7c93426c8bacdab0d7b2
parent 504802 d40fbd0c1a779633e1bd1f0c77cfcbb4f9e6e35a
child 504804 6798cc7aaed0765f259470eb5f7cac16d4310545
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjanerik
bugs1505060
milestone65.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 1505060 - Support multistore for dynamic scalars r=janerik The storage approach for dynamic scalars uses nsAtom to ensure we don't repeatedly store the same string over and over. Differential Revision: https://phabricator.services.mozilla.com/D12788
toolkit/components/telemetry/build_scripts/gen_scalar_data.py
toolkit/components/telemetry/core/ScalarInfo.h
toolkit/components/telemetry/core/TelemetryScalar.cpp
toolkit/components/telemetry/tests/python/test_gen_scalar_data_json.py
toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
toolkit/components/telemetry/tests/unit/test_TelemetryScalars_buildFaster.js
--- a/toolkit/components/telemetry/build_scripts/gen_scalar_data.py
+++ b/toolkit/components/telemetry/build_scripts/gen_scalar_data.py
@@ -142,16 +142,17 @@ def generate_JSON_definitions(output, *f
 
         scalar_definitions[category][scalar.name] = OrderedDict({
             'kind': scalar.nsITelemetry_kind,
             'keyed': scalar.keyed,
             'record_on_release': True if scalar.dataset_short == 'opt-out' else False,
             # We don't expire dynamic-builtin scalars: they're only meant for
             # use in local developer builds anyway. They will expire when rebuilding.
             'expired': False,
+            'stores': scalar.record_into_store,
         })
 
     json.dump(scalar_definitions, output)
 
 
 def main(output, *filenames):
     # Load the scalars first.
     scalars = parse_scalar_definitions(filenames)
--- a/toolkit/components/telemetry/core/ScalarInfo.h
+++ b/toolkit/components/telemetry/core/ScalarInfo.h
@@ -36,25 +36,19 @@ struct BaseScalarInfo {
     , products(aProducts)
     , builtin(aBuiltin)
   {}
   virtual ~BaseScalarInfo() {}
 
   virtual const char *name() const = 0;
   virtual const char *expiration() const = 0;
 
-  virtual uint16_t storeOffset() const
-  {
-    return UINT16_MAX;
-  };
+  virtual uint32_t storeOffset() const = 0;
 
-  virtual uint32_t storeCount() const
-  {
-    return 1;
-  };
+  virtual uint32_t storeCount() const = 0;
 };
 
 /**
  * "Static" scalar definition: these are the ones riding
  * the trains.
  */
 struct ScalarInfo : BaseScalarInfo {
   uint32_t name_offset;
@@ -78,17 +72,17 @@ struct ScalarInfo : BaseScalarInfo {
     , expiration_offset(aExpirationOffset)
     , store_count(aStoreCount)
     , store_offset(aStoreOffset)
   {}
 
   const char *name() const override;
   const char *expiration() const override;
 
-  uint16_t storeOffset() const override
+  uint32_t storeOffset() const override
   {
     return store_offset;
   };
 
   uint32_t storeCount() const override
   {
     return store_count;
   };
--- a/toolkit/components/telemetry/core/TelemetryScalar.cpp
+++ b/toolkit/components/telemetry/core/TelemetryScalar.cpp
@@ -112,16 +112,21 @@ const uint32_t kScalarCount =
 
 // To stop growing unbounded in memory while waiting for scalar deserialization
 // to finish, we immediately apply pending operations if the array reaches
 // a certain high water mark of elements.
 const size_t kScalarActionsArrayHighWaterMark = 10000;
 
 const char* TEST_SCALAR_PREFIX = "telemetry.test.";
 
+// The max offset supported by gScalarStoresTable for static scalars' stores.
+// Also the sentinel value (with store_count == 0) for just the sole "main"
+// store.
+const uint32_t kMaxStaticStoreOffset = UINT16_MAX;
+
 enum class ScalarResult : uint8_t {
   // Nothing went wrong.
   Ok,
   // General Scalar Errors
   NotInitialized,
   CannotUnpackVariant,
   CannotRecordInProcess,
   CannotRecordDataset,
@@ -142,43 +147,65 @@ enum class ScalarResult : uint8_t {
 };
 
 // A common identifier for both built-in and dynamic scalars.
 struct ScalarKey {
   uint32_t id;
   bool dynamic;
 };
 
+// Dynamic scalar store names.
+StaticAutoPtr<nsTArray<RefPtr<nsAtom>>> gDynamicStoreNames;
+
 /**
  * Scalar information for dynamic definitions.
  */
 struct DynamicScalarInfo : BaseScalarInfo {
   nsCString mDynamicName;
   bool mDynamicExpiration;
+  uint32_t store_count;
+  uint32_t store_offset;
 
   DynamicScalarInfo(uint32_t aKind, bool aRecordOnRelease,
                     bool aExpired, const nsACString& aName,
-                    bool aKeyed, bool aBuiltin)
+                    bool aKeyed, bool aBuiltin,
+                    const nsTArray<nsCString>& aStores)
     : BaseScalarInfo(aKind,
                      aRecordOnRelease ?
                      nsITelemetry::DATASET_RELEASE_CHANNEL_OPTOUT :
                      nsITelemetry::DATASET_RELEASE_CHANNEL_OPTIN,
                      RecordedProcessType::All,
                      aKeyed,
                      GetCurrentProduct(),
                      aBuiltin)
     , mDynamicName(aName)
     , mDynamicExpiration(aExpired)
-  {}
+  {
+    store_count = aStores.Length();
+    if (store_count == 0) {
+      store_count = 1;
+      store_offset = kMaxStaticStoreOffset;
+    } else {
+      store_offset = kMaxStaticStoreOffset + 1 + gDynamicStoreNames->Length();
+      for (const auto& storeName: aStores) {
+        gDynamicStoreNames->AppendElement(NS_Atomize(storeName));
+      }
+      MOZ_ASSERT(gDynamicStoreNames->Length() < UINT32_MAX - kMaxStaticStoreOffset - 1,
+                 "Too many dynamic scalar store names. Overflow.");
+    }
+  };
 
   // The following functions will read the stored text
   // instead of looking it up in the statically generated
   // tables.
   const char *name() const override;
   const char *expiration() const override;
+
+  uint32_t storeCount() const override;
+  uint32_t storeOffset() const override;
 };
 
 const char *
 DynamicScalarInfo::name() const
 {
   return mDynamicName.get();
 }
 
@@ -186,16 +213,28 @@ const char *
 DynamicScalarInfo::expiration() const
 {
   // Dynamic scalars can either be expired or not (boolean flag).
   // Return an appropriate version string to leverage the scalar expiration
   // logic.
   return mDynamicExpiration ? "1.0" : "never";
 }
 
+uint32_t
+DynamicScalarInfo::storeOffset() const
+{
+  return store_offset;
+}
+
+uint32_t
+DynamicScalarInfo::storeCount() const
+{
+  return store_count;
+}
+
 typedef nsBaseHashtableET<nsDepCharHashKey, ScalarKey> CharPtrEntryType;
 typedef AutoHashtable<CharPtrEntryType> ScalarMapType;
 
 // Dynamic scalar definitions.
 StaticAutoPtr<nsTArray<DynamicScalarInfo>> gDynamicScalarInfo;
 
 const BaseScalarInfo&
 internal_GetScalarInfo(const StaticMutexAutoLock& lock, const ScalarKey& aId)
@@ -367,17 +406,17 @@ protected:
   void ClearValueInStore(size_t aStoreIndex);
   void SetValueInStores();
   nsresult StoreIndex(const nsACString& aStoreName, size_t* aStoreIndex) const;
 
 private:
   ScalarResult HandleUnsupported() const;
 
   const uint32_t mStoreCount;
-  const uint16_t mStoreOffset;
+  const uint32_t mStoreOffset;
   nsTArray<bool> mStoreHasValue;
 };
 
 ScalarResult
 ScalarBase::HandleUnsupported() const
 {
   MOZ_ASSERT(false, "This operation is not support for this scalar type.");
   return ScalarResult::OperationNotSupported;
@@ -405,26 +444,40 @@ ScalarBase::SetValueInStores()
   for (auto& val: mStoreHasValue) {
     val = true;
   }
 }
 
 nsresult
 ScalarBase::StoreIndex(const nsACString& aStoreName, size_t* aStoreIndex) const
 {
-  if (mStoreCount == 1 && mStoreOffset == UINT16_MAX) {
+  if (mStoreCount == 1 && mStoreOffset == kMaxStaticStoreOffset) {
     // This Scalar is only in the "main" store.
     if (aStoreName.EqualsLiteral("main")) {
       *aStoreIndex = 0;
       return NS_OK;
     }
     return NS_ERROR_NO_CONTENT;
   }
 
   // Multiple stores. Linear scan to find one that matches aStoreName.
+  // Dynamic Scalars start at kMaxStaticStoreOffset + 1
+  if (mStoreOffset > kMaxStaticStoreOffset) {
+    auto dynamicOffset = mStoreOffset - kMaxStaticStoreOffset - 1;
+    for (uint32_t i = 0; i < mStoreCount; ++i) {
+      auto scalarStore = (*gDynamicStoreNames)[dynamicOffset + i];
+      if (nsAtomCString(scalarStore).Equals(aStoreName)) {
+        *aStoreIndex = i;
+        return NS_OK;
+      }
+    }
+    return NS_ERROR_NO_CONTENT;
+  }
+
+  // Static Scalars are similar.
   for (uint32_t i = 0; i < mStoreCount; ++i) {
     uint32_t stringIndex = gScalarStoresTable[mStoreOffset + i];
     if (aStoreName.EqualsASCII(&gScalarsStringTable[stringIndex])) {
       *aStoreIndex = i;
       return NS_OK;
     }
   }
   return NS_ERROR_NO_CONTENT;
@@ -1919,16 +1972,19 @@ internal_BroadcastDefinitions(const Stat
 void
 internal_RegisterScalars(const StaticMutexAutoLock& lock,
                          const nsTArray<DynamicScalarInfo>& scalarInfos)
 {
   // Register the new scalars.
   if (!gDynamicScalarInfo) {
     gDynamicScalarInfo = new nsTArray<DynamicScalarInfo>();
   }
+  if (!gDynamicStoreNames) {
+    gDynamicStoreNames = new nsTArray<RefPtr<nsAtom>>();
+  }
 
   for (auto scalarInfo : scalarInfos) {
     // Allow expiring scalars that were already registered.
     CharPtrEntryType *existingKey = gScalarNameIDMap.GetEntry(scalarInfo.name());
     if (existingKey) {
       // Change the scalar to expired if needed.
       if (scalarInfo.mDynamicExpiration && !scalarInfo.builtin) {
         DynamicScalarInfo& scalarData = (*gDynamicScalarInfo)[existingKey->mData.id];
@@ -2439,16 +2495,17 @@ TelemetryScalar::InitializeGlobalState(b
   const nsTArray<DynamicScalarInfo> initialDynamicScalars({
     DynamicScalarInfo{
       nsITelemetry::SCALAR_TYPE_COUNT,
       true /* recordOnRelease */,
       false /* expired */,
       nsAutoCString("telemetry.dynamic_event_counts"),
       true /* keyed */,
       false /* built-in */,
+      {} /* stores */,
     },
   });
   internal_RegisterScalars(locker, initialDynamicScalars);
 
   gInitDone = true;
 }
 
 void
@@ -2458,16 +2515,17 @@ TelemetryScalar::DeInitializeGlobalState
   gCanRecordBase = false;
   gCanRecordExtended = false;
   gScalarNameIDMap.Clear();
   gScalarStorageMap.Clear();
   gKeyedScalarStorageMap.Clear();
   gDynamicBuiltinScalarStorageMap.Clear();
   gDynamicBuiltinKeyedScalarStorageMap.Clear();
   gDynamicScalarInfo = nullptr;
+  gDynamicStoreNames = nullptr;
   gInitDone = false;
 }
 
 void
 TelemetryScalar::DeserializationStarted()
 {
   StaticMutexAutoLock locker(gTelemetryScalarsMutex);
   gIsDeserializing = true;
@@ -3413,20 +3471,70 @@ TelemetryScalar::RegisterScalars(const n
       if (!JS_GetProperty(cx, scalarDef, "expired", &value) || !value.isBoolean()) {
         JS_ReportErrorASCII(cx, "Invalid 'expired' for scalar %s.",
                             PromiseFlatCString(fullName).get());
         return NS_ERROR_FAILURE;
       }
       expired = static_cast<bool>(value.toBoolean());
     }
 
+    // Get the scalar's optional stores list (default to ["main"]).
+    nsTArray<nsCString> stores;
+    if (JS_HasProperty(cx, scalarDef, "stores", &hasProperty) && hasProperty) {
+      bool isArray = false;
+      if (!JS_GetProperty(cx, scalarDef, "stores", &value)
+          || !JS_IsArrayObject(cx, value, &isArray)
+          || !isArray) {
+        JS_ReportErrorASCII(cx, "Invalid 'stores' for scalar %s.",
+                            PromiseFlatCString(fullName).get());
+        return NS_ERROR_FAILURE;
+      }
+
+      JS::RootedObject arrayObj(cx, &value.toObject());
+      uint32_t storesLength = 0;
+      if (!JS_GetArrayLength(cx, arrayObj, &storesLength)) {
+        JS_ReportErrorASCII(cx, "Can't get 'stores' array length for scalar %s.",
+                            PromiseFlatCString(fullName).get());
+        return NS_ERROR_FAILURE;
+      }
+
+      for (uint32_t i = 0; i < storesLength; ++i) {
+        JS::Rooted<JS::Value> elt(cx);
+        if (!JS_GetElement(cx, arrayObj, i, &elt)) {
+          JS_ReportErrorASCII(cx,
+                              "Can't get element from scalar %s 'stores' array.",
+                              PromiseFlatCString(fullName).get());
+          return NS_ERROR_FAILURE;
+        }
+        if (!elt.isString()) {
+          JS_ReportErrorASCII(cx,
+                              "Element in scalar %s 'stores' array isn't a "
+                              "string.",
+                              PromiseFlatCString(fullName).get());
+          return NS_ERROR_FAILURE;
+        }
+
+        nsAutoJSString jsStr;
+        if (!jsStr.init(cx, elt)) {
+          return NS_ERROR_FAILURE;
+        }
+
+        stores.AppendElement(NS_ConvertUTF16toUTF8(jsStr));
+      }
+      // In the event of the usual case (just "main"), save the storage.
+      if (stores.Length() == 1 && stores[0].EqualsLiteral("main")) {
+        stores.TruncateLength(0);
+      }
+    }
+
     // We defer the actual registration here in case any other event description is invalid.
     // In that case we don't need to roll back any partial registration.
     newScalarInfos.AppendElement(DynamicScalarInfo{
-      kind, recordOnRelease, expired, fullName, keyed, aBuiltin
+      kind, recordOnRelease, expired, fullName, keyed, aBuiltin,
+      std::move(stores)
     });
   }
 
   // Register the dynamic definition on the parent process.
   {
     StaticMutexAutoLock locker(gTelemetryScalarsMutex);
     ::internal_RegisterScalars(locker, newScalarInfos);
 
@@ -3674,17 +3782,18 @@ TelemetryScalar::AddDynamicScalarDefinit
   for (auto def : aDefs) {
     bool recordOnRelease = def.dataset == nsITelemetry::DATASET_RELEASE_CHANNEL_OPTOUT;
     dynamicStubs.AppendElement(DynamicScalarInfo{
       def.type,
       recordOnRelease,
       def.expired,
       def.name,
       def.keyed,
-      false /* builtin */});
+      false /* builtin */,
+      {} /* stores */});
   }
 
   {
     StaticMutexAutoLock locker(gTelemetryScalarsMutex);
     internal_RegisterScalars(locker, dynamicStubs);
   }
 }
 
--- a/toolkit/components/telemetry/tests/python/test_gen_scalar_data_json.py
+++ b/toolkit/components/telemetry/tests/python/test_gen_scalar_data_json.py
@@ -46,23 +46,25 @@ newscalar:
         """
 
         EXPECTED_JSON = {
             "newscalar": {
                 "withoptout": {
                     "kind": "nsITelemetry::SCALAR_TYPE_STRING",
                     "expired": False,
                     "record_on_release": True,
-                    "keyed": False
+                    "keyed": False,
+                    "stores": ["main"],
                 },
                 "withoptin": {
                     "kind": "nsITelemetry::SCALAR_TYPE_COUNT",
                     "expired": False,
                     "record_on_release": False,
-                    "keyed": False
+                    "keyed": False,
+                    "stores": ["main"],
                 }
             }
         }
 
         io = StringIO()
         try:
             tmpfile = tempfile.NamedTemporaryFile(suffix=".json", delete=False)
             # Write the scalar definition to the temporary file
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
@@ -598,16 +598,52 @@ add_task(async function test_dynamicScal
         },
         "invalid_scalar": {
           expired: false,
         },
       },
       "evaluation": /Invalid or missing 'kind'/,
       "description": "No scalar must be registered if the batch contains an invalid one",
     },
+    {
+      "category": "telemetry.test",
+      "data": {
+        "invalid_stores": {
+          kind: Ci.nsITelemetry.SCALAR_TYPE_COUNT,
+          keyed: false,
+          stores: true,
+        },
+      },
+      "evaluation": /Invalid 'stores'/,
+      "description": "Registration must fail if 'stores' is of the wrong type",
+    },
+    {
+      "category": "telemetry.test",
+      "data": {
+        "invalid_stores": {
+          kind: Ci.nsITelemetry.SCALAR_TYPE_COUNT,
+          keyed: false,
+          stores: {},
+        },
+      },
+      "evaluation": /Invalid 'stores'/,
+      "description": "Registration must fail if 'stores' is of the wrong type",
+    },
+    {
+      "category": "telemetry.test",
+      "data": {
+        "invalid_stores": {
+          kind: Ci.nsITelemetry.SCALAR_TYPE_COUNT,
+          keyed: false,
+          stores: [{}],
+        },
+      },
+      "evaluation": /'stores' array isn't a string./,
+      "description": "Registration must fail if element in 'stores' is of the wrong type",
+    },
   ];
 
   for (let testCase of TEST_CASES) {
     Assert.throws(() => Telemetry.registerScalars(testCase.category, testCase.data),
                   testCase.evaluation, testCase.description);
   }
 });
 
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryScalars_buildFaster.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryScalars_buildFaster.js
@@ -58,16 +58,30 @@ add_task({
         "keyed": false,
       },
       "builtin_dynamic_other": {
         "kind": "nsITelemetry::SCALAR_TYPE_BOOLEAN",
         "expired": false,
         "record_on_release": false,
         "keyed": false,
       },
+      "builtin_dynamic_multi": {
+        "kind": "nsITelemetry::SCALAR_TYPE_COUNT",
+        "expired": false,
+        "record_on_release": false,
+        "keyed": false,
+        "stores": ["main", "sync"],
+      },
+      "builtin_dynamic_sync_only": {
+        "kind": "nsITelemetry::SCALAR_TYPE_COUNT",
+        "expired": false,
+        "record_on_release": false,
+        "keyed": false,
+        "stores": ["sync"],
+      },
     },
   };
 
   Telemetry.clearScalars();
 
   // Let's write to the definition file to also cover the file
   // loading part.
   const FILE_PATH = getDefinitionsPath();
@@ -75,34 +89,55 @@ add_task({
 
   // Start TelemetryController to trigger loading the specs.
   await TelemetryController.testReset();
   await TelemetryController.testPromiseJsProbeRegistration();
 
   // Store to that scalar.
   const TEST_SCALAR1 = "telemetry.test.builtin_dynamic";
   const TEST_SCALAR2 = "telemetry.test.builtin_dynamic_other";
+  const TEST_SCALAR3 = "telemetry.test.builtin_dynamic_multi";
+  const TEST_SCALAR4 = "telemetry.test.builtin_dynamic_sync_only";
   Telemetry.scalarSet(TEST_SCALAR1, 3785);
   Telemetry.scalarSet(TEST_SCALAR2, true);
+  Telemetry.scalarSet(TEST_SCALAR3, 1337);
+  Telemetry.scalarSet(TEST_SCALAR4, 31337);
 
   // Check the values we tried to store.
   const scalars = Telemetry.getSnapshotForScalars("main", false).parent;
+  const syncScalars = Telemetry.getSnapshotForScalars("sync", false).parent;
 
   // Check that they are serialized to the correct format.
   Assert.equal(typeof(scalars[TEST_SCALAR1]), "number",
                TEST_SCALAR1 + " must be serialized to the correct format.");
   Assert.ok(Number.isInteger(scalars[TEST_SCALAR1]),
                TEST_SCALAR1 + " must be a finite integer.");
   Assert.equal(scalars[TEST_SCALAR1], 3785,
                TEST_SCALAR1 + " must have the correct value.");
   Assert.equal(typeof(scalars[TEST_SCALAR2]), "boolean",
                TEST_SCALAR2 + " must be serialized to the correct format.");
   Assert.equal(scalars[TEST_SCALAR2], true,
                TEST_SCALAR2 + " must have the correct value.");
 
+  Assert.equal(typeof(scalars[TEST_SCALAR3]), "number",
+               `${TEST_SCALAR3} must be serialized to the correct format.`);
+  Assert.equal(scalars[TEST_SCALAR3], 1337,
+               `${TEST_SCALAR3} must have the correct value.`);
+  Assert.equal(typeof(syncScalars[TEST_SCALAR3]), "number",
+               `${TEST_SCALAR3} must be serialized in the sync store to the correct format.`);
+  Assert.equal(syncScalars[TEST_SCALAR3], 1337,
+               `${TEST_SCALAR3} must have the correct value in the sync snapshot.`);
+
+  Assert.ok(!(TEST_SCALAR4 in scalars),
+            `${TEST_SCALAR4} must not be in the main store.`);
+  Assert.equal(typeof(syncScalars[TEST_SCALAR4]), "number",
+               `${TEST_SCALAR4} must be in the sync snapshot.`);
+  Assert.equal(syncScalars[TEST_SCALAR4], 31337,
+               `${TEST_SCALAR4} must have the correct value.`);
+
   // Clean up.
   await TelemetryController.testShutdown();
   await OS.File.remove(FILE_PATH);
 });
 
 add_task(async function test_keyedDynamicBuiltin() {
   Telemetry.clearScalars();