Skip to content

Commit

Permalink
Remove unnecessary UNDEFINED entry type from PerformanceObserver
Browse files Browse the repository at this point in the history
Summary:
Changelog: [internal]

We were defining an `UNDEFINED` type of entry that should never happen in practice and we were using unnecessarily to signal "no type" where an optional type would be more suitable. Most importantly, **we were incorrectly allocating a buffer for entries of this type**.

This removes that type and the unnecessary buffer.

Differential Revision: D55478890
  • Loading branch information
rubennorte authored and facebook-github-bot committed Mar 28, 2024
1 parent 4311644 commit 5aadbdf
Show file tree
Hide file tree
Showing 6 changed files with 32 additions and 40 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@ std::vector<RawPerformanceEntry> NativePerformanceObserver::getEntries(
std::optional<int32_t> entryType,
std::optional<std::string> entryName) {
return PerformanceEntryReporter::getInstance().getEntries(
entryType ? static_cast<PerformanceEntryType>(*entryType)
: PerformanceEntryType::UNDEFINED,
entryType ? std::optional{static_cast<PerformanceEntryType>(*entryType)}
: std::nullopt,
entryName ? entryName->c_str() : nullptr);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ void PerformanceEntryReporter::logEntry(const RawPerformanceEntry& entry) {

std::lock_guard lock(entriesMutex_);

auto& buffer = buffers_[entry.entryType];
auto& buffer = getBuffer(entryType);

if (entry.duration < buffer.durationThreshold) {
// The entries duration is lower than the desired reporting threshold, skip
Expand Down Expand Up @@ -161,15 +161,15 @@ void PerformanceEntryReporter::mark(
}

void PerformanceEntryReporter::clearEntries(
PerformanceEntryType entryType,
std::optional<PerformanceEntryType> entryType,
std::string_view entryName) {
if (entryType == PerformanceEntryType::UNDEFINED) {
if (!entryType) {
// Clear all entry types
for (int i = 1; i < NUM_PERFORMANCE_ENTRY_TYPES; i++) {
clearEntries(static_cast<PerformanceEntryType>(i), entryName);
}
} else {
auto& buffer = getBuffer(entryType);
auto& buffer = getBuffer(*entryType);
if (!entryName.empty()) {
if (buffer.hasNameLookup) {
std::lock_guard lock2(nameLookupMutex_);
Expand Down Expand Up @@ -208,29 +208,29 @@ void PerformanceEntryReporter::getEntries(
PerformanceEntryType entryType,
std::string_view entryName,
std::vector<RawPerformanceEntry>& res) const {
if (entryType == PerformanceEntryType::UNDEFINED) {
// Collect all entry types
for (int i = 1; i < NUM_PERFORMANCE_ENTRY_TYPES; i++) {
getEntries(static_cast<PerformanceEntryType>(i), entryName, res);
}
std::lock_guard lock(entriesMutex_);
const auto& entries = getBuffer(entryType).entries;
if (entryName.empty()) {
entries.getEntries(res);
} else {
std::lock_guard lock(entriesMutex_);
const auto& entries = getBuffer(entryType).entries;
if (entryName.empty()) {
entries.getEntries(res);
} else {
entries.getEntries(res, [entryName](const RawPerformanceEntry& entry) {
return entry.name == entryName;
});
}
entries.getEntries(res, [entryName](const RawPerformanceEntry& entry) {
return entry.name == entryName;
});
}
}

std::vector<RawPerformanceEntry> PerformanceEntryReporter::getEntries(
PerformanceEntryType entryType,
std::optional<PerformanceEntryType> entryType,
std::string_view entryName) const {
std::vector<RawPerformanceEntry> res;
getEntries(entryType, entryName, res);
if (!entryType) {
// Collect all entry types
for (int i = 1; i < NUM_PERFORMANCE_ENTRY_TYPES; i++) {
getEntries(static_cast<PerformanceEntryType>(i), entryName, res);
}
} else {
getEntries(*entryType, entryName, res);
}
return res;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,15 +59,14 @@ struct PerformanceEntryBuffer {
};

enum class PerformanceEntryType {
UNDEFINED = 0,
MARK = 1,
MEASURE = 2,
EVENT = 3,
_COUNT = 4,
_NEXT = 4,
};

constexpr size_t NUM_PERFORMANCE_ENTRY_TYPES =
(size_t)PerformanceEntryType::_COUNT;
(size_t)PerformanceEntryType::_NEXT - 1; // Valid types start from 1.

class PerformanceEntryReporter : public EventLogger, public UIManagerMountHook {
public:
Expand All @@ -94,12 +93,12 @@ class PerformanceEntryReporter : public EventLogger, public UIManagerMountHook {
void logEntry(const RawPerformanceEntry& entry);

PerformanceEntryBuffer& getBuffer(PerformanceEntryType entryType) {
return buffers_[static_cast<int>(entryType)];
return buffers_[static_cast<int>(entryType) - 1];
}

const PerformanceEntryBuffer& getBuffer(
PerformanceEntryType entryType) const {
return buffers_[static_cast<int>(entryType)];
return buffers_[static_cast<int>(entryType) - 1];
}

bool isReporting(PerformanceEntryType entryType) const {
Expand Down Expand Up @@ -127,11 +126,11 @@ class PerformanceEntryReporter : public EventLogger, public UIManagerMountHook {
const std::optional<std::string>& endMark = std::nullopt);

void clearEntries(
PerformanceEntryType entryType = PerformanceEntryType::UNDEFINED,
std::optional<PerformanceEntryType> entryType = std::nullopt,
std::string_view entryName = {});

std::vector<RawPerformanceEntry> getEntries(
PerformanceEntryType entryType = PerformanceEntryType::UNDEFINED,
std::optional<PerformanceEntryType> entryType = std::nullopt,
std::string_view entryName = {}) const;

void logEventEntry(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,7 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestGetEntries) {
const auto marks = reporter.getEntries(PerformanceEntryType::MARK);

const auto measures = reporter.getEntries(PerformanceEntryType::MEASURE);
const auto common_name =
reporter.getEntries(PerformanceEntryType::UNDEFINED, "common_name");
const auto common_name = reporter.getEntries(std::nullopt, "common_name");

reporter.getEntries();
const auto all = reporter.getEntries();
Expand Down Expand Up @@ -304,7 +303,7 @@ TEST(PerformanceEntryReporter, PerformanceEntryReporterTestClearEntries) {
reporter.measure("measure3", 0.0, 0.0, 5.0, "mark1");
reporter.measure("measure4", 1.5, 0.0, std::nullopt, std::nullopt, "mark2");

reporter.clearEntries(PerformanceEntryType::UNDEFINED, "common_name");
reporter.clearEntries(std::nullopt, "common_name");
auto e1 = reporter.getEntries();

ASSERT_EQ(6, e1.size());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {PerformanceEntry} from './PerformanceEntry';
import PerformanceEventTiming from './PerformanceEventTiming';

export const RawPerformanceEntryTypeValues = {
UNDEFINED: 0,
MARK: 1,
MEASURE: 2,
EVENT: 3,
Expand Down Expand Up @@ -56,10 +55,6 @@ export function rawToPerformanceEntryType(
return 'measure';
case RawPerformanceEntryTypeValues.EVENT:
return 'event';
case RawPerformanceEntryTypeValues.UNDEFINED:
throw new TypeError(
"rawToPerformanceEntryType: UNDEFINED can't be cast to PerformanceEntryType",
);
default:
throw new TypeError(
`rawToPerformanceEntryType: unexpected performance entry type received: ${type}`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,10 @@ const NativePerformanceObserverMock: NativePerformanceObserver = {
durationThresholds.set(entryType, durationThreshold);
},

clearEntries: (entryType: RawPerformanceEntryType, entryName?: string) => {
clearEntries: (entryType?: RawPerformanceEntryType, entryName?: string) => {
entries = entries.filter(
e =>
(entryType !== RawPerformanceEntryTypeValues.UNDEFINED &&
e.entryType !== entryType) ||
(entryType != null && e.entryType !== entryType) ||
(entryName != null && e.name !== entryName),
);
},
Expand Down

0 comments on commit 5aadbdf

Please sign in to comment.