Skip to content

Commit

Permalink
Cancel dropped log events
Browse files Browse the repository at this point in the history
Summary:
APIs may require that events are properly released instead of just dropped.
I recently added some restrictions to when MOUNT events, in particular, are
logged and this led to a bunch of events never getting properly canceled/released.

This adds the necessary interface to the ComponentsLogger and implements it
where necessary.

Reviewed By: IanChilds

Differential Revision: D9447798

fbshipit-source-id: 106191d9aabf0ccc8db098ae8edc93e3386d795a
  • Loading branch information
passy authored and facebook-github-bot committed Aug 23, 2018
1 parent 4c545ad commit d3117ef
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ enum LogLevel {
/** Write a {@link PerfEvent} to storage. This also marks the end of the event. */
void logPerfEvent(PerfEvent event);

/** Release a previously obtained {@link PerfEvent} without logging it. */
void cancelPerfEvent(PerfEvent event);

/**
* Emit a message that can be logged or escalated by the logger implementation.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public static PerfEvent populatePerfEventFromLogger(
ComponentContext c, ComponentsLogger logger, PerfEvent perfEvent) {
final String logTag = c.getLogTag();
if (logTag == null) {
logger.cancelPerfEvent(perfEvent);
return null;
}

Expand Down
9 changes: 6 additions & 3 deletions litho-core/src/main/java/com/facebook/litho/MountState.java
Original file line number Diff line number Diff line change
Expand Up @@ -386,22 +386,25 @@ && canMountIncrementally(component)) {

suppressInvalidationsOnHosts(false);

logMountPerfEvent(componentTree, logger, mountPerfEvent);
if (logger != null) {
logMountPerfEvent(logger, mountPerfEvent);
}

if (isTracing) {
ComponentsSystrace.endSection();
}
}

private void logMountPerfEvent(
ComponentTree componentTree, ComponentsLogger logger, @Nullable PerfEvent mountPerfEvent) {
private void logMountPerfEvent(ComponentsLogger logger, @Nullable PerfEvent mountPerfEvent) {
if (!mMountStats.isLoggingEnabled || mountPerfEvent == null) {
logger.cancelPerfEvent(mountPerfEvent);
return;
}

// MOUNT events that don't mount any content are not valuable enough to log at the moment.
// We will likely enable them again in the future. T31729233
if (mMountStats.mountedCount == 0) {
logger.cancelPerfEvent(mountPerfEvent);
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public Map<String, String> getExtraAnnotations(TreeProps treeProps) {

@Test
public void testSkipOnEmptyTag() {
final BaseComponentsLogger logger =
final TestComponentsLogger logger =
new TestComponentsLogger() {
@Nullable
@Override
Expand All @@ -91,7 +91,11 @@ public Map<String, String> getExtraAnnotations(TreeProps treeProps) {
mContext.setTreeProps(treeProps);

final ComponentContext noLogTagContext = new ComponentContext(RuntimeEnvironment.application);
LogTreePopulator.populatePerfEventFromLogger(noLogTagContext, logger, event);
final PerfEvent perfEvent =
LogTreePopulator.populatePerfEventFromLogger(noLogTagContext, logger, event);

assertThat(perfEvent).isNull();
assertThat(logger.getCanceledPerfEvents()).containsExactly(event);

verifyNoMoreInteractions(event);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
public class TestComponentsLogger extends BaseComponentsLogger {

private final List<PerfEvent> mLoggedPerfEvents = new LinkedList<>();
private final List<PerfEvent> mCanceledPerfEvents = new LinkedList<>();
private final List<Pair<LogLevel, String>> mLoggedMessages = new LinkedList<>();

@Override
Expand All @@ -43,6 +44,11 @@ public PerfEvent newPerformanceEvent(@FrameworkLogEvents.LogEventId int eventId)
return new TestPerfEvent(eventId);
}

@Override
public void cancelPerfEvent(PerfEvent event) {
mCanceledPerfEvents.add(event);
}

@Override
public void logPerfEvent(PerfEvent event) {
mLoggedPerfEvents.add(event);
Expand All @@ -62,6 +68,10 @@ public List<PerfEvent> getLoggedPerfEvents() {
return mLoggedPerfEvents;
}

public List<PerfEvent> getCanceledPerfEvents() {
return mCanceledPerfEvents;
}

public List<Pair<LogLevel, String>> getLoggedMessages() {
return mLoggedMessages;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ class SampleComponentsLogger : BaseComponentsLogger() {
printEventData(event, PerfEventStore.release(event))
}

override fun cancelPerfEvent(event: PerfEvent) {
PerfEventStore.release(event)
}

override fun emitMessage(level: ComponentsLogger.LogLevel, message: String) {
when (level) {
ComponentsLogger.LogLevel.WARNING -> {
Expand Down

0 comments on commit d3117ef

Please sign in to comment.