diff --git a/toolkit/components/telemetry/TelemetryScalar.cpp b/toolkit/components/telemetry/TelemetryScalar.cpp index b8297a377343..a8e7465478fc 100644 --- a/toolkit/components/telemetry/TelemetryScalar.cpp +++ b/toolkit/components/telemetry/TelemetryScalar.cpp @@ -34,8 +34,6 @@ using mozilla::Preferences; using mozilla::StaticAutoPtr; using mozilla::StaticMutex; using mozilla::StaticMutexAutoLock; -using mozilla::Some; -using mozilla::Nothing; using mozilla::Telemetry::Common::AutoHashtable; using mozilla::Telemetry::Common::IsExpiredVersion; using mozilla::Telemetry::Common::CanRecordDataset; @@ -49,8 +47,6 @@ using mozilla::Telemetry::Common::IsValidIdentifierString; using mozilla::Telemetry::Common::GetCurrentProduct; using mozilla::Telemetry::Common::SupportedProduct; using mozilla::Telemetry::ScalarActionType; -using mozilla::Telemetry::ScalarAction; -using mozilla::Telemetry::KeyedScalarAction; using mozilla::Telemetry::ScalarID; using mozilla::Telemetry::DynamicScalarDefinition; using mozilla::Telemetry::ScalarVariant; @@ -109,11 +105,6 @@ const uint32_t kMaximumScalarNameLength = 40; const uint32_t kScalarCount = static_cast(mozilla::Telemetry::ScalarID::ScalarCount); -// 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; - enum class ScalarResult : uint8_t { // Nothing went wrong. Ok, @@ -937,20 +928,6 @@ ProcessesKeyedScalarsMapType gKeyedScalarStorageMap; ProcessesScalarsMapType gDynamicBuiltinScalarStorageMap; ProcessesKeyedScalarsMapType gDynamicBuiltinKeyedScalarStorageMap; -// Whether or not the deserialization of persisted scalars is still in progress. -// This is never the case on Desktop or Fennec. -// Only GeckoView restores persisted scalars. -bool gIsDeserializing = false; -// This batches scalar accumulations that should be applied once loading finished. -StaticAutoPtr> gScalarsActions; -StaticAutoPtr> gKeyedScalarsActions; - -bool -internal_IsScalarDeserializing(const StaticMutexAutoLock& lock) -{ - return gIsDeserializing; -} - } // namespace //////////////////////////////////////////////////////////////////////// @@ -1267,115 +1244,6 @@ internal_GetScalarByEnum(const StaticMutexAutoLock& lock, return NS_OK; } -void internal_ApplyPendingOperations(const StaticMutexAutoLock& lock); - -/** - * Record the given action on a scalar into the pending actions list. - * - * If the pending actions list overflows the high water mark length - * all operations are immediately applied, including the passed action. - * - * @param aScalarAction The action to record. - */ -void -internal_RecordScalarAction(const StaticMutexAutoLock& lock, - const ScalarAction& aScalarAction) -{ - // Make sure to have the storage. - if (!gScalarsActions) { - gScalarsActions = new nsTArray(); - } - - // Store the action. - gScalarsActions->AppendElement(aScalarAction); - - // If this action overflows the pending actions array, we immediately apply pending operations - // and assume loading is over. - // If loading still happens afterwards, some scalar values might be - // overwritten and inconsistent, but we won't lose operations on otherwise untouched probes. - if (gScalarsActions->Length() > kScalarActionsArrayHighWaterMark) { - internal_ApplyPendingOperations(lock); - return; - } -} - -/** - * Record the given action on a scalar on the main process into the pending actions list. - * - * If the pending actions list overflows the high water mark length - * all operations are immediately applied, including the passed action. - * - * @param aId The scalar's ID this action applies to - * @param aDynamic Determines if the scalar is dynamic - * @param aAction The action to record - * @param aValue The additional data for the recorded action - */ -void -internal_RecordScalarAction(const StaticMutexAutoLock& lock, - uint32_t aId, bool aDynamic, - ScalarActionType aAction, const ScalarVariant& aValue) -{ - internal_RecordScalarAction(lock, ScalarAction{ - aId, aDynamic, aAction, - Some(aValue), ProcessID::Parent - }); -} - -/** - * Record the given action on a keyed scalar into the pending actions list. - * - * If the pending actions list overflows the high water mark length - * all operations are immediately applied, including the passed action. - * - * @param aScalarAction The action to record. - */ -void -internal_RecordKeyedScalarAction(const StaticMutexAutoLock& lock, - const KeyedScalarAction& aScalarAction) -{ - // Make sure to have the storage. - if (!gKeyedScalarsActions) { - gKeyedScalarsActions = new nsTArray(); - } - - // Store the action. - gKeyedScalarsActions->AppendElement(aScalarAction); - - // If this action overflows the pending actions array, we immediately apply pending operations - // and assume loading is over. - // If loading still happens afterwards, some scalar values might be - // overwritten and inconsistent, but we won't lose operations on otherwise untouched probes. - if (gKeyedScalarsActions->Length() > kScalarActionsArrayHighWaterMark) { - internal_ApplyPendingOperations(lock); - return; - } -} - -/** - * Record the given action on a keyed scalar on the main process into the pending actions list. - * - * If the pending actions list overflows the high water mark length - * all operations are immediately applied, including the passed action. - * - * @param aId The scalar's ID this action applies to - * @param aDynamic Determines if the scalar is dynamic - * @param aKey The scalar's key - * @param aAction The action to record - * @param aValue The additional data for the recorded action - */ -void -internal_RecordKeyedScalarAction(const StaticMutexAutoLock& lock, - uint32_t aId, bool aDynamic, - const nsAString& aKey, - ScalarActionType aAction, - const ScalarVariant& aValue) -{ - internal_RecordKeyedScalarAction(lock, KeyedScalarAction{ - aId, aDynamic, aAction, NS_ConvertUTF16toUTF8(aKey), - Some(aValue), ProcessID::Parent - }); -} - /** * Update the scalar with the provided value. This is used by the JS API. * @@ -1386,14 +1254,12 @@ internal_RecordKeyedScalarAction(const StaticMutexAutoLock& lock, * @param aProcessOverride The process for which the scalar must be updated. * This must only be used for GeckoView persistence. It must be * set to the ProcessID::Parent for all the other cases. - * @param aForce Whether to force updating even if load is in progress. * @return a ScalarResult error value. */ ScalarResult internal_UpdateScalar(const StaticMutexAutoLock& lock, const nsACString& aName, ScalarActionType aType, nsIVariant* aValue, - ProcessID aProcessOverride = ProcessID::Parent, - bool aForce = false) + ProcessID aProcessOverride = ProcessID::Parent) { ScalarKey uniqueId; nsresult rv = internal_GetEnumByScalarName(lock, aName, &uniqueId); @@ -1425,19 +1291,6 @@ internal_UpdateScalar(const StaticMutexAutoLock& lock, const nsACString& aName, return ScalarResult::Ok; } - if (!aForce && internal_IsScalarDeserializing(lock)) { - const BaseScalarInfo &info = internal_GetScalarInfo(lock, uniqueId); - // Convert the nsIVariant to a Variant. - mozilla::Maybe variantValue; - sr = GetVariantFromIVariant(aValue, info.kind, variantValue); - if (sr != ScalarResult::Ok) { - MOZ_ASSERT(false, "Unable to convert nsIVariant to mozilla::Variant."); - return sr; - } - internal_RecordScalarAction(lock, uniqueId.id, uniqueId.dynamic, aType, variantValue.ref()); - return ScalarResult::Ok; - } - // Finally get the scalar. ScalarBase* scalar = nullptr; rv = internal_GetScalarByEnum(lock, uniqueId, aProcessOverride, &scalar); @@ -1570,8 +1423,7 @@ ScalarResult internal_UpdateKeyedScalar(const StaticMutexAutoLock& lock, const nsACString& aName, const nsAString& aKey, ScalarActionType aType, nsIVariant* aValue, - ProcessID aProcessOverride = ProcessID::Parent, - bool aForce = false) + ProcessID aProcessOverride = ProcessID::Parent) { ScalarKey uniqueId; nsresult rv = internal_GetEnumByScalarName(lock, aName, &uniqueId); @@ -1603,21 +1455,6 @@ internal_UpdateKeyedScalar(const StaticMutexAutoLock& lock, return ScalarResult::Ok; } - if (!aForce && internal_IsScalarDeserializing(lock)) { - const BaseScalarInfo &info = internal_GetScalarInfo(lock, uniqueId); - // Convert the nsIVariant to a Variant. - mozilla::Maybe variantValue; - sr = GetVariantFromIVariant(aValue, info.kind, variantValue); - if (sr != ScalarResult::Ok) { - MOZ_ASSERT(false, "Unable to convert nsIVariant to mozilla::Variant."); - return sr; - } - internal_RecordKeyedScalarAction(lock, - uniqueId.id, uniqueId.dynamic, - aKey, aType, variantValue.ref()); - return ScalarResult::Ok; - } - // Finally get the scalar. KeyedScalar* scalar = nullptr; rv = internal_GetKeyedScalarByEnum(lock, uniqueId, aProcessOverride, &scalar); @@ -1900,219 +1737,6 @@ internal_GetKeyedScalarSnapshot(const StaticMutexAutoLock& aLock, } // namespace -// helpers for recording/applying scalar operations -namespace { - -void -internal_ApplyScalarActions(const StaticMutexAutoLock& lock, - const nsTArray& aScalarActions, - const mozilla::Maybe& aProcessType = Nothing()) -{ - if (!internal_CanRecordBase(lock)) { - return; - } - - for (auto& upd : aScalarActions) { - ScalarKey uniqueId{upd.mId, upd.mDynamic}; - if (NS_WARN_IF(!internal_IsValidId(lock, uniqueId))) { - MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids."); - continue; - } - - if (internal_IsKeyedScalar(lock, uniqueId)) { - continue; - } - - // Are we allowed to record this scalar? We don't need to check for - // allowed processes here, that's taken care of when recording - // in child processes. - if (!internal_CanRecordForScalarID(lock, uniqueId)) { - continue; - } - - // Either we got passed a process type or it was explicitely set on the recorded action. - // It should never happen that it is set to an invalid value (such as ProcessID::Count) - ProcessID processType = aProcessType.valueOr(upd.mProcessType); - MOZ_ASSERT(processType != ProcessID::Count); - - // Refresh the data in the parent process with the data coming from the child - // processes. - ScalarBase* scalar = nullptr; - nsresult rv = internal_GetScalarByEnum(lock, uniqueId, processType, - &scalar); - if (NS_FAILED(rv)) { - NS_WARNING("NS_FAILED internal_GetScalarByEnum for CHILD"); - continue; - } - - if (upd.mData.isNothing()) { - MOZ_ASSERT(false, "There is no data in the ScalarActionType."); - continue; - } - - // Get the type of this scalar from the scalar ID. We already checked - // for its validity a few lines above. - const uint32_t scalarType = internal_GetScalarInfo(lock, uniqueId).kind; - - // Extract the data from the mozilla::Variant. - switch (upd.mActionType) - { - case ScalarActionType::eSet: - { - switch (scalarType) - { - case nsITelemetry::SCALAR_TYPE_COUNT: - scalar->SetValue(upd.mData->as()); - break; - case nsITelemetry::SCALAR_TYPE_BOOLEAN: - scalar->SetValue(upd.mData->as()); - break; - case nsITelemetry::SCALAR_TYPE_STRING: - scalar->SetValue(upd.mData->as()); - break; - } - break; - } - case ScalarActionType::eAdd: - { - if (scalarType != nsITelemetry::SCALAR_TYPE_COUNT) { - NS_WARNING("Attempting to add on a non count scalar."); - continue; - } - // We only support adding uint32_t. - scalar->AddValue(upd.mData->as()); - break; - } - case ScalarActionType::eSetMaximum: - { - if (scalarType != nsITelemetry::SCALAR_TYPE_COUNT) { - NS_WARNING("Attempting to add on a non count scalar."); - continue; - } - // We only support SetMaximum on uint32_t. - scalar->SetMaximum(upd.mData->as()); - break; - } - default: - NS_WARNING("Unsupported action coming from scalar child updates."); - } - } -} - -void -internal_ApplyKeyedScalarActions(const StaticMutexAutoLock& lock, - const nsTArray& aScalarActions, - const mozilla::Maybe& aProcessType = Nothing()) -{ - if (!internal_CanRecordBase(lock)) { - return; - } - - for (auto& upd : aScalarActions) { - ScalarKey uniqueId{upd.mId, upd.mDynamic}; - if (NS_WARN_IF(!internal_IsValidId(lock, uniqueId))) { - MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids."); - continue; - } - - if (!internal_IsKeyedScalar(lock, uniqueId)) { - continue; - } - - // Are we allowed to record this scalar? We don't need to check for - // allowed processes here, that's taken care of when recording - // in child processes. - if (!internal_CanRecordForScalarID(lock, uniqueId)) { - continue; - } - - // Either we got passed a process type or it was explicitely set on the recorded action. - // It should never happen that it is set to an invalid value (such as ProcessID::Count) - ProcessID processType = aProcessType.valueOr(upd.mProcessType); - MOZ_ASSERT(processType != ProcessID::Count); - - // Refresh the data in the parent process with the data coming from the child - // processes. - KeyedScalar* scalar = nullptr; - nsresult rv = internal_GetKeyedScalarByEnum(lock, uniqueId, processType, - &scalar); - if (NS_FAILED(rv)) { - NS_WARNING("NS_FAILED internal_GetScalarByEnum for CHILD"); - continue; - } - - if (upd.mData.isNothing()) { - MOZ_ASSERT(false, "There is no data in the KeyedScalarAction."); - continue; - } - - // Get the type of this scalar from the scalar ID. We already checked - // for its validity a few lines above. - const uint32_t scalarType = internal_GetScalarInfo(lock, uniqueId).kind; - - // Extract the data from the mozilla::Variant. - switch (upd.mActionType) - { - case ScalarActionType::eSet: - { - switch (scalarType) - { - case nsITelemetry::SCALAR_TYPE_COUNT: - scalar->SetValue(NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as()); - break; - case nsITelemetry::SCALAR_TYPE_BOOLEAN: - scalar->SetValue(NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as()); - break; - default: - NS_WARNING("Unsupported type coming from scalar child updates."); - } - break; - } - case ScalarActionType::eAdd: - { - if (scalarType != nsITelemetry::SCALAR_TYPE_COUNT) { - NS_WARNING("Attempting to add on a non count scalar."); - continue; - } - // We only support adding on uint32_t. - scalar->AddValue(NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as()); - break; - } - case ScalarActionType::eSetMaximum: - { - if (scalarType != nsITelemetry::SCALAR_TYPE_COUNT) { - NS_WARNING("Attempting to add on a non count scalar."); - continue; - } - // We only support SetMaximum on uint32_t. - scalar->SetMaximum(NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as()); - break; - } - default: - NS_WARNING("Unsupported action coming from keyed scalar child updates."); - } - } -} - -void -internal_ApplyPendingOperations(const StaticMutexAutoLock& lock) -{ - if (gScalarsActions && gScalarsActions->Length() > 0) { - internal_ApplyScalarActions(lock, *gScalarsActions); - gScalarsActions->Clear(); - } - - if (gKeyedScalarsActions && gKeyedScalarsActions->Length() > 0) { - internal_ApplyKeyedScalarActions(lock, *gKeyedScalarsActions); - gKeyedScalarsActions->Clear(); - } - - // After all pending operations are applied deserialization is done - gIsDeserializing = false; -} - -} // namespace - //////////////////////////////////////////////////////////////////////// //////////////////////////////////////////////////////////////////////// // @@ -2179,20 +1803,6 @@ TelemetryScalar::DeInitializeGlobalState() gInitDone = false; } -void -TelemetryScalar::DeserializationStarted() -{ - StaticMutexAutoLock locker(gTelemetryScalarsMutex); - gIsDeserializing = true; -} - -void -TelemetryScalar::ApplyPendingOperations() -{ - StaticMutexAutoLock locker(gTelemetryScalarsMutex); - internal_ApplyPendingOperations(locker); -} - void TelemetryScalar::SetCanRecordBase(bool b) { @@ -2311,13 +1921,6 @@ TelemetryScalar::Add(mozilla::Telemetry::ScalarID aId, uint32_t aValue) return; } - if (internal_IsScalarDeserializing(locker)) { - internal_RecordScalarAction(locker, uniqueId.id, uniqueId.dynamic, - ScalarActionType::eAdd, - ScalarVariant(aValue)); - return; - } - ScalarBase* scalar = nullptr; nsresult rv = internal_GetScalarByEnum(locker, uniqueId, ProcessID::Parent, &scalar); @@ -2359,14 +1962,6 @@ TelemetryScalar::Add(mozilla::Telemetry::ScalarID aId, const nsAString& aKey, return; } - if (internal_IsScalarDeserializing(locker)) { - internal_RecordKeyedScalarAction(locker, uniqueId.id, uniqueId.dynamic, - aKey, - ScalarActionType::eAdd, - ScalarVariant(aValue)); - return; - } - KeyedScalar* scalar = nullptr; nsresult rv = internal_GetKeyedScalarByEnum(locker, uniqueId, ProcessID::Parent, @@ -2483,13 +2078,6 @@ TelemetryScalar::Set(mozilla::Telemetry::ScalarID aId, uint32_t aValue) return; } - if (internal_IsScalarDeserializing(locker)) { - internal_RecordScalarAction(locker, uniqueId.id, uniqueId.dynamic, - ScalarActionType::eSet, - ScalarVariant(aValue)); - return; - } - ScalarBase* scalar = nullptr; nsresult rv = internal_GetScalarByEnum(locker, uniqueId, ProcessID::Parent, &scalar); @@ -2530,13 +2118,6 @@ TelemetryScalar::Set(mozilla::Telemetry::ScalarID aId, const nsAString& aValue) return; } - if (internal_IsScalarDeserializing(locker)) { - internal_RecordScalarAction(locker, uniqueId.id, uniqueId.dynamic, - ScalarActionType::eSet, - ScalarVariant(nsString(aValue))); - return; - } - ScalarBase* scalar = nullptr; nsresult rv = internal_GetScalarByEnum(locker, uniqueId, ProcessID::Parent, &scalar); @@ -2577,13 +2158,6 @@ TelemetryScalar::Set(mozilla::Telemetry::ScalarID aId, bool aValue) return; } - if (internal_IsScalarDeserializing(locker)) { - internal_RecordScalarAction(locker, uniqueId.id, uniqueId.dynamic, - ScalarActionType::eSet, - ScalarVariant(aValue)); - return; - } - ScalarBase* scalar = nullptr; nsresult rv = internal_GetScalarByEnum(locker, uniqueId, ProcessID::Parent, &scalar); @@ -2625,14 +2199,6 @@ TelemetryScalar::Set(mozilla::Telemetry::ScalarID aId, const nsAString& aKey, return; } - if (internal_IsScalarDeserializing(locker)) { - internal_RecordKeyedScalarAction(locker, uniqueId.id, uniqueId.dynamic, - aKey, - ScalarActionType::eSet, - ScalarVariant(aValue)); - return; - } - KeyedScalar* scalar = nullptr; nsresult rv = internal_GetKeyedScalarByEnum(locker, uniqueId, ProcessID::Parent, @@ -2675,14 +2241,6 @@ TelemetryScalar::Set(mozilla::Telemetry::ScalarID aId, const nsAString& aKey, return; } - if (internal_IsScalarDeserializing(locker)) { - internal_RecordKeyedScalarAction(locker, uniqueId.id, uniqueId.dynamic, - aKey, - ScalarActionType::eSet, - ScalarVariant(aValue)); - return; - } - KeyedScalar* scalar = nullptr; nsresult rv = internal_GetKeyedScalarByEnum(locker, uniqueId, ProcessID::Parent, @@ -2799,13 +2357,6 @@ TelemetryScalar::SetMaximum(mozilla::Telemetry::ScalarID aId, uint32_t aValue) return; } - if (internal_IsScalarDeserializing(locker)) { - internal_RecordScalarAction(locker, uniqueId.id, uniqueId.dynamic, - ScalarActionType::eSetMaximum, - ScalarVariant(aValue)); - return; - } - ScalarBase* scalar = nullptr; nsresult rv = internal_GetScalarByEnum(locker, uniqueId, ProcessID::Parent, &scalar); @@ -2847,14 +2398,6 @@ TelemetryScalar::SetMaximum(mozilla::Telemetry::ScalarID aId, const nsAString& a return; } - if (internal_IsScalarDeserializing(locker)) { - internal_RecordKeyedScalarAction(locker, uniqueId.id, uniqueId.dynamic, - aKey, - ScalarActionType::eSetMaximum, - ScalarVariant(aValue)); - return; - } - KeyedScalar* scalar = nullptr; nsresult rv = internal_GetKeyedScalarByEnum(locker, uniqueId, ProcessID::Parent, &scalar); @@ -3218,8 +2761,6 @@ TelemetryScalar::ClearScalars() gKeyedScalarStorageMap.Clear(); gDynamicBuiltinScalarStorageMap.Clear(); gDynamicBuiltinKeyedScalarStorageMap.Clear(); - gScalarsActions = nullptr; - gKeyedScalarsActions = nullptr; } size_t @@ -3264,22 +2805,90 @@ TelemetryScalar::UpdateChildData(ProcessID aProcessType, MOZ_ASSERT(XRE_IsParentProcess(), "The stored child processes scalar data must be updated from the parent process."); StaticMutexAutoLock locker(gTelemetryScalarsMutex); - - // If scalars are still being deserialized, we need to record the incoming - // operations as well. - if (internal_IsScalarDeserializing(locker)) { - for (const ScalarAction& action : aScalarActions) { - // We're only getting immutable access, so let's copy it - ScalarAction copy = action; - // Fix up the process type - copy.mProcessType = aProcessType; - internal_RecordScalarAction(locker, copy); - } - + if (!internal_CanRecordBase(locker)) { return; } - internal_ApplyScalarActions(locker, aScalarActions, Some(aProcessType)); + for (auto& upd : aScalarActions) { + ScalarKey uniqueId{upd.mId, upd.mDynamic}; + if (NS_WARN_IF(!internal_IsValidId(locker, uniqueId))) { + MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids."); + continue; + } + + if (internal_IsKeyedScalar(locker, uniqueId)) { + continue; + } + + // Are we allowed to record this scalar? We don't need to check for + // allowed processes here, that's taken care of when recording + // in child processes. + if (!internal_CanRecordForScalarID(locker, uniqueId)) { + continue; + } + + // Refresh the data in the parent process with the data coming from the child + // processes. + ScalarBase* scalar = nullptr; + nsresult rv = internal_GetScalarByEnum(locker, uniqueId, aProcessType, + &scalar); + if (NS_FAILED(rv)) { + NS_WARNING("NS_FAILED internal_GetScalarByEnum for CHILD"); + continue; + } + + if (upd.mData.isNothing()) { + MOZ_ASSERT(false, "There is no data in the ScalarActionType."); + continue; + } + + // Get the type of this scalar from the scalar ID. We already checked + // for its validity a few lines above. + const uint32_t scalarType = internal_GetScalarInfo(locker, uniqueId).kind; + + // Extract the data from the mozilla::Variant. + switch (upd.mActionType) + { + case ScalarActionType::eSet: + { + switch (scalarType) + { + case nsITelemetry::SCALAR_TYPE_COUNT: + scalar->SetValue(upd.mData->as()); + break; + case nsITelemetry::SCALAR_TYPE_BOOLEAN: + scalar->SetValue(upd.mData->as()); + break; + case nsITelemetry::SCALAR_TYPE_STRING: + scalar->SetValue(upd.mData->as()); + break; + } + break; + } + case ScalarActionType::eAdd: + { + if (scalarType != nsITelemetry::SCALAR_TYPE_COUNT) { + NS_WARNING("Attempting to add on a non count scalar."); + continue; + } + // We only support adding uint32_t. + scalar->AddValue(upd.mData->as()); + break; + } + case ScalarActionType::eSetMaximum: + { + if (scalarType != nsITelemetry::SCALAR_TYPE_COUNT) { + NS_WARNING("Attempting to add on a non count scalar."); + continue; + } + // We only support SetMaximum on uint32_t. + scalar->SetMaximum(upd.mData->as()); + break; + } + default: + NS_WARNING("Unsupported action coming from scalar child updates."); + } + } } void @@ -3289,22 +2898,89 @@ TelemetryScalar::UpdateChildKeyedData(ProcessID aProcessType, MOZ_ASSERT(XRE_IsParentProcess(), "The stored child processes keyed scalar data must be updated from the parent process."); StaticMutexAutoLock locker(gTelemetryScalarsMutex); - - // If scalars are still being deserialized, we need to record the incoming - // operations as well. - if (internal_IsScalarDeserializing(locker)) { - for (const KeyedScalarAction& action : aScalarActions) { - // We're only getting immutable access, so let's copy it - KeyedScalarAction copy = action; - // Fix up the process type - copy.mProcessType = aProcessType; - internal_RecordKeyedScalarAction(locker, copy); - } - + if (!internal_CanRecordBase(locker)) { return; } - internal_ApplyKeyedScalarActions(locker, aScalarActions, Some(aProcessType)); + for (auto& upd : aScalarActions) { + ScalarKey uniqueId{upd.mId, upd.mDynamic}; + if (NS_WARN_IF(!internal_IsValidId(locker, uniqueId))) { + MOZ_ASSERT_UNREACHABLE("Scalar usage requires valid ids."); + continue; + } + + if (!internal_IsKeyedScalar(locker, uniqueId)) { + continue; + } + + // Are we allowed to record this scalar? We don't need to check for + // allowed processes here, that's taken care of when recording + // in child processes. + if (!internal_CanRecordForScalarID(locker, uniqueId)) { + continue; + } + + // Refresh the data in the parent process with the data coming from the child + // processes. + KeyedScalar* scalar = nullptr; + nsresult rv = internal_GetKeyedScalarByEnum(locker, uniqueId, aProcessType, + &scalar); + if (NS_FAILED(rv)) { + NS_WARNING("NS_FAILED internal_GetScalarByEnum for CHILD"); + continue; + } + + if (upd.mData.isNothing()) { + MOZ_ASSERT(false, "There is no data in the KeyedScalarAction."); + continue; + } + + // Get the type of this scalar from the scalar ID. We already checked + // for its validity a few lines above. + const uint32_t scalarType = internal_GetScalarInfo(locker, uniqueId).kind; + + // Extract the data from the mozilla::Variant. + switch (upd.mActionType) + { + case ScalarActionType::eSet: + { + switch (scalarType) + { + case nsITelemetry::SCALAR_TYPE_COUNT: + scalar->SetValue(NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as()); + break; + case nsITelemetry::SCALAR_TYPE_BOOLEAN: + scalar->SetValue(NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as()); + break; + default: + NS_WARNING("Unsupported type coming from scalar child updates."); + } + break; + } + case ScalarActionType::eAdd: + { + if (scalarType != nsITelemetry::SCALAR_TYPE_COUNT) { + NS_WARNING("Attempting to add on a non count scalar."); + continue; + } + // We only support adding on uint32_t. + scalar->AddValue(NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as()); + break; + } + case ScalarActionType::eSetMaximum: + { + if (scalarType != nsITelemetry::SCALAR_TYPE_COUNT) { + NS_WARNING("Attempting to add on a non count scalar."); + continue; + } + // We only support SetMaximum on uint32_t. + scalar->SetMaximum(NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as()); + break; + } + default: + NS_WARNING("Unsupported action coming from keyed scalar child updates."); + } + } } void @@ -3649,8 +3325,7 @@ TelemetryScalar::DeserializePersistedScalars(JSContext* aCx, JS::HandleValue aDa processScalars[i].first(), ScalarActionType::eSet, processScalars[i].second(), - ProcessID(iter.Key()), - true /* aForce */); + ProcessID(iter.Key())); } } } @@ -3822,8 +3497,7 @@ TelemetryScalar::DeserializePersistedKeyedScalars(JSContext* aCx, JS::HandleValu mozilla::Get<1>(processScalars[i]), ScalarActionType::eSet, mozilla::Get<2>(processScalars[i]), - ProcessID(iter.Key()), - true /* aForce */); + ProcessID(iter.Key())); } } } diff --git a/toolkit/components/telemetry/TelemetryScalar.h b/toolkit/components/telemetry/TelemetryScalar.h index 113c8cc2d00b..dcc3708ed7b5 100644 --- a/toolkit/components/telemetry/TelemetryScalar.h +++ b/toolkit/components/telemetry/TelemetryScalar.h @@ -96,11 +96,6 @@ nsresult SerializeScalars(mozilla::JSONWriter &aWriter); nsresult SerializeKeyedScalars(mozilla::JSONWriter &aWriter); nsresult DeserializePersistedScalars(JSContext* aCx, JS::HandleValue aData); nsresult DeserializePersistedKeyedScalars(JSContext* aCx, JS::HandleValue aData); -// Mark deserialization as in progress. -// After this, all scalar operations are recorded into the pending operations list. -void DeserializationStarted(); -// Apply all operations from the pending operations list and mark deserialization finished afterwards. -void ApplyPendingOperations(); } // namespace TelemetryScalar #endif // TelemetryScalar_h__ diff --git a/toolkit/components/telemetry/docs/internals/geckoview.rst b/toolkit/components/telemetry/docs/internals/geckoview.rst index 8161584920cf..5e41f54bb8e4 100644 --- a/toolkit/components/telemetry/docs/internals/geckoview.rst +++ b/toolkit/components/telemetry/docs/internals/geckoview.rst @@ -41,34 +41,6 @@ measurements file after the update interval expires. This interval is defined by ``toolkit.telemetry.geckoPersistenceTimeout`` preference (defaults to 1 minute), see the :doc:`preferences docs `. -Scalar Semantics -~~~~~~~~~~~~~~~~ - -Data collection can start very early during the application life cycle and might overlap with the persistence deserialization. -The Telemetry Core therefore takes additional steps to preserve the semantics of scalar operations. - -During deserialization of persisted measurements operations on scalar probes are not applied immediately, but recorded into a pending operations list. -Once deserialization is finished, the pending operations are applied in the order they were recorded. -This avoids any data race between operations and scalar values are always in a consistent state. -Snapshots requests will only be fulfilled after the deserialization finished and all pending operations are applied. -Consumers of the recorded data should therefore never see inconsistent data. - -An example: - -1. Scalar deserialization is started -2. "test" scalar is incremented by "10" by the application -> The operation ``[test, add, 10]`` is recorded into the list. -3. The state of the "test" scalar is loaded off the persistence file, and the value "14" is set. -4. Deserialization is finished and the pending operations are applied. - - * The "test" scalar is incremented by "10", the value is now "24" -5. "test" scalar is incremented via "scalarAdd" by 1. Its value is "25" - -To stop growing unbounded in memory while waiting for scalar deserialization to finish, pending operations are applied -immediately if the array reaches a high water mark of 10000 elements. -At that point the deserialization is considered done and following scalar operatins will be applied immediately. -In the case of the deserialization still being in progress, it might overwrite recorded values, -leading to inconsistent data. - The persistence file format --------------------------- All the supported measurements are serialized to the persistence file using the JSON format. @@ -174,4 +146,3 @@ Version history - Initial GeckoView support and scalar persistence (`bug 1453591 `_). - Persistence support for histograms (`bug 1457127 `_). - - Preserve the semantics of scalar operations when restoring the persisted state (`bug 1454606 `_) diff --git a/toolkit/components/telemetry/geckoview/TelemetryGeckoViewPersistence.cpp b/toolkit/components/telemetry/geckoview/TelemetryGeckoViewPersistence.cpp index 94b8d8a43ed5..a6e79c7afd54 100644 --- a/toolkit/components/telemetry/geckoview/TelemetryGeckoViewPersistence.cpp +++ b/toolkit/components/telemetry/geckoview/TelemetryGeckoViewPersistence.cpp @@ -446,9 +446,6 @@ PersistenceThreadLoadData() if (!fileContent.IsEmpty()) { MainThreadParsePersistedProbes(fileContent); } - - TelemetryScalar::ApplyPendingOperations(); - // Arm the timer. MainThreadArmPersistenceTimer(); // Notify that we're good to take snapshots! @@ -516,9 +513,6 @@ TelemetryGeckoViewPersistence::InitPersistence() gPersistenceThread = thread.forget(); - // From now on all scalar operations should be recorded. - TelemetryScalar::DeserializationStarted(); - // Trigger the loading of the persistence data. After the function // completes it will automatically arm the persistence timer. gPersistenceThread->Dispatch( diff --git a/toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp b/toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp index fa8b072b38b3..096346b763c0 100644 --- a/toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp +++ b/toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp @@ -475,173 +475,3 @@ TEST_F(TelemetryGeckoViewFixture, TimerHitCountProbe) { // Cleanup/remove the files. RemovePersistenceFile(); } - -TEST_F(TelemetryGeckoViewFixture, EmptyPendingOperations) { - AutoJSContextWithGlobal cx(mCleanGlobal); - - Unused << mTelemetry->ClearScalars(); - - // Force loading mode - TelemetryScalar::DeserializationStarted(); - - // Do nothing explicitely - - // Force pending operations to be applied and end load mode. - // It should not crash and don't change any scalars. - TelemetryScalar::ApplyPendingOperations(); - - // Check that the snapshot is empty - JS::RootedValue scalarsSnapshot(cx.GetJSContext()); - GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot); - - JS::RootedObject scalarsSnapshotObj(cx.GetJSContext(), &scalarsSnapshot.toObject()); - ASSERT_TRUE(scalarsSnapshot.isUndefined()) << "Scalars snapshot should not contain any data."; -} - -TEST_F(TelemetryGeckoViewFixture, SimpleAppendOperation) { - AutoJSContextWithGlobal cx(mCleanGlobal); - - Unused << mTelemetry->ClearScalars(); - - // Set an initial value, so we can test that it is not overwritten. - uint32_t initialValue = 1; - Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, initialValue); - - // Force loading mode - TelemetryScalar::DeserializationStarted(); - - // Add to a scalar - uint32_t value = 37; - Telemetry::ScalarAdd(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, value); - - // Verify that this was not yet applied. - JS::RootedValue scalarsSnapshot(cx.GetJSContext()); - GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot); - - CheckUintScalar("telemetry.test.unsigned_int_kind", cx.GetJSContext(), scalarsSnapshot, initialValue); - - // Force pending operations to be applied and end load mode - TelemetryScalar::ApplyPendingOperations(); - - // Verify recorded operations are applied - GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot); - CheckUintScalar("telemetry.test.unsigned_int_kind", cx.GetJSContext(), scalarsSnapshot, initialValue+value); -} - -TEST_F(TelemetryGeckoViewFixture, ApplyPendingOperationsAfterLoad) { - AutoJSContextWithGlobal cx(mCleanGlobal); - - Unused << mTelemetry->ClearScalars(); - - const char persistedData[] = R"({ - "scalars": { - "parent": { - "telemetry.test.unsigned_int_kind": 14 - } - } -})"; - - // Force loading mode - TelemetryScalar::DeserializationStarted(); - - // Add to a scalar, this should be recorded - uint32_t addValue = 10; - Telemetry::ScalarAdd(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, addValue); - - // Load persistence file - RefPtr loadingFinished = new DataLoadedObserver(); - WritePersistenceFile(nsDependentCString(persistedData)); - TelemetryGeckoViewPersistence::InitPersistence(); - loadingFinished->WaitForNotification(); - - // At this point all pending operations should have been applied. - - // Increment again, now directly applied - uint32_t val = 1; - Telemetry::ScalarAdd(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, val); - - - JS::RootedValue scalarsSnapshot(cx.GetJSContext()); - GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot); - - uint32_t expectedValue = 25; - CheckUintScalar("telemetry.test.unsigned_int_kind", cx.GetJSContext(), scalarsSnapshot, expectedValue); -} - -TEST_F(TelemetryGeckoViewFixture, MultipleAppendOperations) { - AutoJSContextWithGlobal cx(mCleanGlobal); - - Unused << mTelemetry->ClearScalars(); - - // Force loading mode - TelemetryScalar::DeserializationStarted(); - - // Modify all kinds of scalars - uint32_t startValue = 35; - uint32_t expectedValue = 40; - Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, startValue); - Telemetry::ScalarSetMaximum(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, startValue + 2); - Telemetry::ScalarAdd(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, 3); - - Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_BOOLEAN_KIND, true); - Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_STRING_KIND, NS_LITERAL_STRING("Star Wars VI")); - - // Modify all kinds of keyed scalars - Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_KEYED_UNSIGNED_INT, - NS_LITERAL_STRING("chewbacca"), startValue); - Telemetry::ScalarSetMaximum(Telemetry::ScalarID::TELEMETRY_TEST_KEYED_UNSIGNED_INT, - NS_LITERAL_STRING("chewbacca"), startValue + 2); - Telemetry::ScalarAdd(Telemetry::ScalarID::TELEMETRY_TEST_KEYED_UNSIGNED_INT, - NS_LITERAL_STRING("chewbacca"), 3); - - Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_KEYED_BOOLEAN_KIND, - NS_LITERAL_STRING("chewbacca"), - true); - - // Force pending operations to be applied and end load mode - TelemetryScalar::ApplyPendingOperations(); - - JS::RootedValue scalarsSnapshot(cx.GetJSContext()); - JS::RootedValue keyedScalarsSnapshot(cx.GetJSContext()); - GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot); - GetScalarsSnapshot(true, cx.GetJSContext(), &keyedScalarsSnapshot); - - CheckUintScalar("telemetry.test.unsigned_int_kind", cx.GetJSContext(), scalarsSnapshot, expectedValue); - CheckBoolScalar("telemetry.test.boolean_kind", cx.GetJSContext(), scalarsSnapshot, true); - CheckStringScalar("telemetry.test.string_kind", cx.GetJSContext(), scalarsSnapshot, "Star Wars VI"); - - CheckKeyedUintScalar("telemetry.test.keyed_unsigned_int", "chewbacca", - cx.GetJSContext(), keyedScalarsSnapshot, expectedValue); - CheckKeyedBoolScalar("telemetry.test.keyed_boolean_kind", "chewbacca", - cx.GetJSContext(), keyedScalarsSnapshot, true); -} - -TEST_F(TelemetryGeckoViewFixture, PendingOperationsHighWater) { - AutoJSContextWithGlobal cx(mCleanGlobal); - - Unused << mTelemetry->ClearScalars(); - - uint32_t initialValue = 0; - Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, initialValue); - - // Force loading mode - TelemetryScalar::DeserializationStarted(); - - // Fill up the pending operations list - uint32_t expectedValue = 10000; - for (uint32_t i=0; i < expectedValue; i++) { - Telemetry::ScalarAdd(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, 1); - } - - // Nothing should be applied yet - JS::RootedValue scalarsSnapshot(cx.GetJSContext()); - GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot); - CheckUintScalar("telemetry.test.unsigned_int_kind", cx.GetJSContext(), scalarsSnapshot, 0); - - // Spill over the buffer to immediately apply all operations - Telemetry::ScalarAdd(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, 1); - - // Now we should see all values - GetScalarsSnapshot(false, cx.GetJSContext(), &scalarsSnapshot); - CheckUintScalar("telemetry.test.unsigned_int_kind", cx.GetJSContext(), scalarsSnapshot, expectedValue+1); -} diff --git a/toolkit/components/telemetry/ipc/TelemetryComms.h b/toolkit/components/telemetry/ipc/TelemetryComms.h index c8f48c649f5b..dd7524616174 100644 --- a/toolkit/components/telemetry/ipc/TelemetryComms.h +++ b/toolkit/components/telemetry/ipc/TelemetryComms.h @@ -10,7 +10,6 @@ #include "nsITelemetry.h" #include "nsVariant.h" #include "mozilla/TimeStamp.h" -#include "mozilla/TelemetryProcessEnums.h" namespace mozilla { namespace Telemetry { @@ -50,9 +49,6 @@ struct ScalarAction // We need to wrap mData in a Maybe otherwise the IPC system // is unable to instantiate a ScalarAction. Maybe mData; - // The process type this scalar should be recorded for. - // The IPC system will determine the process this action was coming from later. - mozilla::Telemetry::ProcessID mProcessType; }; struct KeyedScalarAction @@ -64,9 +60,6 @@ struct KeyedScalarAction // We need to wrap mData in a Maybe otherwise the IPC system // is unable to instantiate a ScalarAction. Maybe mData; - // The process type this scalar should be recorded for. - // The IPC system will determine the process this action was coming from later. - mozilla::Telemetry::ProcessID mProcessType; }; // Dynamic scalars support. diff --git a/toolkit/components/telemetry/ipc/TelemetryIPCAccumulator.cpp b/toolkit/components/telemetry/ipc/TelemetryIPCAccumulator.cpp index 7e357feb17e5..d5006bcfe94f 100644 --- a/toolkit/components/telemetry/ipc/TelemetryIPCAccumulator.cpp +++ b/toolkit/components/telemetry/ipc/TelemetryIPCAccumulator.cpp @@ -191,8 +191,8 @@ TelemetryIPCAccumulator::RecordChildScalarAction(uint32_t aId, bool aDynamic, if (gChildScalarsActions->Length() == kScalarActionsArrayHighWaterMark) { DispatchIPCTimerFired(); } - // Store the action. The ProcessID will be determined by the receiver. - gChildScalarsActions->AppendElement(ScalarAction{aId, aDynamic, aAction, Some(aValue), Telemetry::ProcessID::Count}); + // Store the action. + gChildScalarsActions->AppendElement(ScalarAction{aId, aDynamic, aAction, Some(aValue)}); ArmIPCTimer(locker); } @@ -215,9 +215,9 @@ TelemetryIPCAccumulator::RecordChildKeyedScalarAction(uint32_t aId, bool aDynami if (gChildKeyedScalarsActions->Length() == kScalarActionsArrayHighWaterMark) { DispatchIPCTimerFired(); } - // Store the action. The ProcessID will be determined by the receiver. + // Store the action. gChildKeyedScalarsActions->AppendElement( - KeyedScalarAction{aId, aDynamic, aAction, NS_ConvertUTF16toUTF8(aKey), Some(aValue), Telemetry::ProcessID::Count}); + KeyedScalarAction{aId, aDynamic, aAction, NS_ConvertUTF16toUTF8(aKey), Some(aValue)}); ArmIPCTimer(locker); }