Skip to content

Commit

Permalink
BREAKING: [litho] Update calculate layout QPL marker to use spans ins…
Browse files Browse the repository at this point in the history 
…tead of separate events

Summary: Previously we had a calculate_layout QPL marker as well as ones for create_layout, css_layout, and collect_results. Spans are a better way to represent this because logging the events isn't free (and they all seem to have different sample rates).

Reviewed By: passy

Differential Revision: D15663192

fbshipit-source-id: f620d5c2d25121b913940e0a865495abdcbe026e
  • Loading branch information
@astreet @facebook-github-bot
astreet authored and facebook-github-bot committed Jun 15, 2019
1 parent 510d19c commit b859605
Show file tree
Hide file tree
Showing 4 changed files with 55 additions and 79 deletions.
9 changes: 3 additions & 6 deletions litho-core/src/main/java/com/facebook/litho/FrameworkLogEvents.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@
* supplied to the {@link ComponentContext} used to create the tree.
*/
public interface FrameworkLogEvents {
int EVENT_CREATE_LAYOUT = 0;
int EVENT_CSS_LAYOUT = 1;
int EVENT_COLLECT_RESULTS = 2;
// Previously int EVENT_CREATE_LAYOUT = 0; Now unused.
// Previously int EVENT_CSS_LAYOUT = 1; Now unused.
// Previously int EVENT_COLLECT_RESULTS = 2; Now unused.
int EVENT_LAYOUT_CALCULATE = 3;
// Previously int EVENT_PREPARE_PART_DEFINITION = 4; Now unused.
int EVENT_PREPARE_MOUNT = 5;
Expand All @@ -47,9 +47,6 @@ public interface FrameworkLogEvents {
int EVENT_RESUME_CALCULATE_LAYOUT_STATE = 19;

@IntDef({
FrameworkLogEvents.EVENT_CREATE_LAYOUT,
FrameworkLogEvents.EVENT_CSS_LAYOUT,
FrameworkLogEvents.EVENT_COLLECT_RESULTS,
FrameworkLogEvents.EVENT_LAYOUT_CALCULATE,
FrameworkLogEvents.EVENT_PREPARE_MOUNT,
FrameworkLogEvents.EVENT_MOUNT,
Expand Down
110 changes: 46 additions & 64 deletions litho-core/src/main/java/com/facebook/litho/LayoutState.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,9 @@
import static com.facebook.litho.ComponentLifecycle.MountType.NONE;
import static com.facebook.litho.ContextUtils.getValidActivityForContext;
import static com.facebook.litho.FrameworkLogEvents.EVENT_CALCULATE_LAYOUT_STATE;
import static com.facebook.litho.FrameworkLogEvents.EVENT_COLLECT_RESULTS;
import static com.facebook.litho.FrameworkLogEvents.EVENT_CREATE_LAYOUT;
import static com.facebook.litho.FrameworkLogEvents.EVENT_CSS_LAYOUT;
import static com.facebook.litho.FrameworkLogEvents.EVENT_RESUME_CALCULATE_LAYOUT_STATE;
import static com.facebook.litho.FrameworkLogEvents.PARAM_COMPONENT;
import static com.facebook.litho.FrameworkLogEvents.PARAM_LAYOUT_STATE_SOURCE;
import static com.facebook.litho.FrameworkLogEvents.PARAM_ROOT_COMPONENT;
import static com.facebook.litho.FrameworkLogEvents.PARAM_TREE_DIFF_ENABLED;
import static com.facebook.litho.MountItem.LAYOUT_FLAG_DISABLE_TOUCHABLE;
import static com.facebook.litho.MountItem.LAYOUT_FLAG_DUPLICATE_PARENT_STATE;
Expand Down Expand Up @@ -1371,6 +1367,8 @@ static LayoutState calculate(
.flush();
}

final DiffNode diffTreeRoot =
currentLayoutState != null ? currentLayoutState.mDiffTreeRoot : null;
final LayoutState layoutState;
try {
final PerfEvent logLayoutState =
Expand All @@ -1381,6 +1379,7 @@ static LayoutState calculate(
if (logLayoutState != null) {
logLayoutState.markerAnnotate(PARAM_COMPONENT, component.getSimpleName());
logLayoutState.markerAnnotate(PARAM_LAYOUT_STATE_SOURCE, sourceToString(source));
logLayoutState.markerAnnotate(PARAM_TREE_DIFF_ENABLED, diffTreeRoot != null);
}

// Detect errors internal to components
Expand Down Expand Up @@ -1412,7 +1411,8 @@ static LayoutState calculate(
heightSpec,
null, // nestedTreeHolder is null as this is measuring the root component tree.
isReconcilable ? currentLayoutState.mLayoutRoot : null,
currentLayoutState != null ? currentLayoutState.mDiffTreeRoot : null)
diffTreeRoot,
logLayoutState)
: layoutCreatedInWillRender;

layoutState.mLayoutRoot = root;
Expand All @@ -1424,12 +1424,16 @@ static LayoutState calculate(
return layoutState;
}

if (logLayoutState != null) {
logLayoutState.markerPoint("start_collect_results");
}

setSizeAfterMeasureAndCollectResults(c, layoutState);

if (logLayoutState != null) {
logLayoutState.markerPoint("end_collect_results");
logger.logPerfEvent(logLayoutState);
}

} finally {
if (isTracing) {
ComponentsSystrace.endSection();
Expand Down Expand Up @@ -1497,7 +1501,8 @@ static LayoutState resumeCalculate(
heightSpec,
null, // nestedTreeHolder is null as this is measuring the root component tree.)
layoutState.mLayoutRoot,
layoutState.mDiffTreeRoot);
layoutState.mDiffTreeRoot,
logLayoutState);

layoutState.mIsPartialLayoutState = false;

Expand All @@ -1520,7 +1525,6 @@ static LayoutState resumeCalculate(

private static void setSizeAfterMeasureAndCollectResults(
ComponentContext c, LayoutState layoutState) {
ComponentsLogger logger = c.getLogger();
final boolean isTracing = ComponentsSystrace.isTracing();
final int widthSpec = layoutState.mWidthSpec;
final int heightSpec = layoutState.mHeightSpec;
Expand Down Expand Up @@ -1559,12 +1563,6 @@ private static void setSizeAfterMeasureAndCollectResults(
return;
}

final PerfEvent collectResultsEvent =
logger != null
? LogTreePopulator.populatePerfEventFromLogger(
c, logger, logger.newPerformanceEvent(c, EVENT_COLLECT_RESULTS))
: null;

collectResults(c, root, layoutState, null);

if (isTracing) {
Expand All @@ -1576,12 +1574,6 @@ private static void setSizeAfterMeasureAndCollectResults(
ComponentsSystrace.endSection();
}

if (collectResultsEvent != null) {
collectResultsEvent.markerAnnotate(
FrameworkLogEvents.PARAM_ROOT_COMPONENT, root.getTailComponent().getSimpleName());
logger.logPerfEvent(collectResultsEvent);
}

if (!c.isReconciliationEnabled()
&& !ComponentsConfiguration.isDebugModeEnabled
&& !ComponentsConfiguration.isEndToEndTestRun
Expand Down Expand Up @@ -1712,31 +1704,11 @@ private void calculateAndSetVisibilityOutputId(
@VisibleForTesting
static InternalNode createTree(
Component component, ComponentContext context, @Nullable InternalNode current) {

final ComponentsLogger logger = context.getLogger();
final PerfEvent createLayoutPerfEvent =
logger != null
? LogTreePopulator.populatePerfEventFromLogger(
context, logger, logger.newPerformanceEvent(context, EVENT_CREATE_LAYOUT))
: null;

if (createLayoutPerfEvent != null) {
createLayoutPerfEvent.markerAnnotate(PARAM_COMPONENT, component.getSimpleName());
}

final InternalNode root;

if (current != null) {
root = current.reconcile(context, component);
} else {
root = component.createLayout(context, true /* resolveNestedTree */);
}

if (createLayoutPerfEvent != null) {
logger.logPerfEvent(createLayoutPerfEvent);
return current.reconcile(context, component);
}

return root;
return component.createLayout(context, true /* resolveNestedTree */);
}

@VisibleForTesting
Expand All @@ -1745,8 +1717,6 @@ static void measureTree(
int widthSpec,
int heightSpec,
DiffNode previousDiffTreeRoot) {
final ComponentContext context = root.getContext();
final Component component = root.getTailComponent();
final boolean isTracing = ComponentsSystrace.isTracing();

if (isTracing) {
Expand All @@ -1766,18 +1736,6 @@ static void measureTree(
ComponentsSystrace.endSection(/* applyDiffNode */);
}

final ComponentsLogger logger = context.getLogger();
final PerfEvent layoutEvent =
logger != null
? LogTreePopulator.populatePerfEventFromLogger(
context, logger, logger.newPerformanceEvent(context, EVENT_CSS_LAYOUT))
: null;

if (layoutEvent != null) {
layoutEvent.markerAnnotate(PARAM_TREE_DIFF_ENABLED, previousDiffTreeRoot != null);
layoutEvent.markerAnnotate(PARAM_ROOT_COMPONENT, root.getTailComponent().getSimpleName());
}

root.calculateLayout(
SizeSpec.getMode(widthSpec) == SizeSpec.UNSPECIFIED
? YogaConstants.UNDEFINED
Expand All @@ -1786,10 +1744,6 @@ static void measureTree(
? YogaConstants.UNDEFINED
: SizeSpec.getSize(heightSpec));

if (layoutEvent != null) {
logger.logPerfEvent(layoutEvent);
}

if (isTracing) {
ComponentsSystrace.endSection(/* measureTree */ );
}
Expand Down Expand Up @@ -1860,7 +1814,8 @@ static InternalNode resolveNestedTree(
heightSpec,
holder,
null,
holder.getDiffNode()); // Was set while traversing the holder's tree.
holder.getDiffNode(), // Was set while traversing the holder's tree.
null);
}

resolvedLayout.setLastWidthSpec(widthSpec);
Expand Down Expand Up @@ -1900,7 +1855,8 @@ static InternalNode createAndMeasureTreeForComponent(
Component component,
int widthSpec,
int heightSpec) {
return createAndMeasureTreeForComponent(c, component, widthSpec, heightSpec, null, null, null);
return createAndMeasureTreeForComponent(
c, component, widthSpec, heightSpec, null, null, null, null);
}

@VisibleForTesting
Expand All @@ -1911,7 +1867,12 @@ static InternalNode createAndMeasureTreeForComponent(
int heightSpec,
@Nullable InternalNode nestedTreeHolder, // Will be set iff resolving a nested tree.
@Nullable InternalNode current,
@Nullable DiffNode diffTreeRoot) {
@Nullable DiffNode diffTreeRoot,
@Nullable PerfEvent layoutStatePerfEvent) {

if (layoutStatePerfEvent != null) {
layoutStatePerfEvent.markerPoint("start_create_layout");
}

component.updateInternalChildState(c);

Expand Down Expand Up @@ -1946,6 +1907,10 @@ static InternalNode createAndMeasureTreeForComponent(
c.setWidthSpec(previousWidthSpec);
c.setHeightSpec(previousHeightSpec);

if (layoutStatePerfEvent != null) {
layoutStatePerfEvent.markerPoint("end_create_layout");
}

if (root == NULL_LAYOUT || c.wasLayoutInterrupted()) {
return root;
}
Expand All @@ -1961,12 +1926,20 @@ static InternalNode createAndMeasureTreeForComponent(
root.layoutDirection(YogaDirection.RTL);
}

if (layoutStatePerfEvent != null) {
layoutStatePerfEvent.markerPoint("start_measure");
}

measureTree(
root,
widthSpec,
heightSpec,
diffTreeRoot);

if (layoutStatePerfEvent != null) {
layoutStatePerfEvent.markerPoint("end_measure");
}

return root;
}

Expand All @@ -1977,7 +1950,8 @@ private static InternalNode resumeCreateAndMeasureTreeForComponent(
int heightSpec,
@Nullable InternalNode nestedTreeHolder, // Will be set iff resolving a nested tree.
InternalNode root,
@Nullable DiffNode diffTreeRoot) {
@Nullable DiffNode diffTreeRoot,
@Nullable PerfEvent logLayoutState) {
resumeCreateTree(c, root);

if (root == NULL_LAYOUT) {
Expand All @@ -1995,8 +1969,16 @@ private static InternalNode resumeCreateAndMeasureTreeForComponent(
root.layoutDirection(YogaDirection.RTL);
}

if (logLayoutState != null) {
logLayoutState.markerPoint("start_measure");
}

measureTree(root, widthSpec, heightSpec, diffTreeRoot);

if (logLayoutState != null) {
logLayoutState.markerPoint("end_measure");
}

return root;
}

Expand Down
12 changes: 6 additions & 6 deletions litho-it-powermock/src/test/java/com/facebook/litho/ComponentLifecycleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ public void testOnShouldCreateLayoutWithNewSizeSpec_shouldUseCache() {

when(LayoutState.resolveNestedTree(mContext, holder, 100, 100)).thenCallRealMethod();
when(LayoutState.createAndMeasureTreeForComponent(
mContext, component, 100, 100, holder, null, null))
mContext, component, 100, 100, holder, null, null, null))
.thenReturn(resolved);

// call resolve nested tree 1st time
Expand All @@ -311,7 +311,7 @@ public void testOnShouldCreateLayoutWithNewSizeSpec_shouldUseCache() {
PowerMockito.verifyStatic();

// it should call create and measure
LayoutState.createAndMeasureTreeForComponent(mContext, component, 100, 100, holder, null, null);
LayoutState.createAndMeasureTreeForComponent(mContext, component, 100, 100, holder, null, null, null);

// should return nested tree next time
when(holder.getNestedTree()).thenReturn(result);
Expand All @@ -324,7 +324,7 @@ public void testOnShouldCreateLayoutWithNewSizeSpec_shouldUseCache() {

// no new invocation of create
PowerMockito.verifyStatic(times(1));
LayoutState.createAndMeasureTreeForComponent(mContext, component, 100, 100, holder, null, null);
LayoutState.createAndMeasureTreeForComponent(mContext, component, 100, 100, holder, null, null, null);

// should only measure
PowerMockito.verifyStatic(times(1));
Expand Down Expand Up @@ -356,7 +356,7 @@ public void testOnShouldCreateLayoutWithNewSizeSpec_shouldNotUseCache() {

when(LayoutState.resolveNestedTree(mContext, holder, 100, 100)).thenCallRealMethod();
when(LayoutState.createAndMeasureTreeForComponent(
mContext, component, 100, 100, holder, null, null))
mContext, component, 100, 100, holder, null, null, null))
.thenReturn(resolved);

// call resolve nested tree 1st time
Expand All @@ -365,7 +365,7 @@ public void testOnShouldCreateLayoutWithNewSizeSpec_shouldNotUseCache() {
PowerMockito.verifyStatic();

// it should call create and measure
LayoutState.createAndMeasureTreeForComponent(mContext, component, 100, 100, holder, null, null);
LayoutState.createAndMeasureTreeForComponent(mContext, component, 100, 100, holder, null, null, null);

// should return nested tree next time
when(holder.getNestedTree()).thenReturn(result);
Expand All @@ -378,7 +378,7 @@ public void testOnShouldCreateLayoutWithNewSizeSpec_shouldNotUseCache() {

// a new invocation of create
PowerMockito.verifyStatic(times(2));
LayoutState.createAndMeasureTreeForComponent(mContext, component, 100, 100, holder, null, null);
LayoutState.createAndMeasureTreeForComponent(mContext, component, 100, 100, holder, null, null, null);

ComponentsConfiguration.enableShouldCreateLayoutWithNewSizeSpec = false;
ComponentsConfiguration.isNestedTreeResolutionExperimentEnabled = false;
Expand Down
3 changes: 0 additions & 3 deletions sample-kotlin/src/main/java/com/fblitho/lithoktsample/logging/SampleComponentsLogger.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,6 @@ class SampleComponentsLogger : BaseComponentsLogger() {

private fun getEventNameById(@FrameworkLogEvents.LogEventId markerId: Int): String =
when (markerId) {
FrameworkLogEvents.EVENT_CREATE_LAYOUT -> "CREATE_LAYOUT"
FrameworkLogEvents.EVENT_CSS_LAYOUT -> "CSS_LAYOUT"
FrameworkLogEvents.EVENT_COLLECT_RESULTS -> "COLLECT_RESULTS"
FrameworkLogEvents.EVENT_LAYOUT_CALCULATE -> "LAYOUT_CALCULATE"
FrameworkLogEvents.EVENT_PREPARE_MOUNT -> "PREPARE_MOUNT"
FrameworkLogEvents.EVENT_MOUNT -> "MOUNT"
Expand Down

0 comments on commit b859605

Please sign in to comment.