Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VirtualizedList function optimization #20208

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 85 additions & 6 deletions Libraries/Lists/VirtualizedList.js
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,65 @@ type ChildListState = {

type State = {first: number, last: number};

/**
* Performs equality by iterating through keys on an object and returning false
* when any key has values which are not strictly equal between the arguments.
* Returns true when the values of all keys are strictly equal.
*/
function shallowEqual(objA: Object, objB: Object, exceptions = []) {
if (is(objA, objB)) {
return true;
}

if (
typeof objA !== 'object' ||
objA === null ||
typeof objB !== 'object' ||
objB === null
) {
return false;
}

var keysA = Object.keys(objA);
var keysB = Object.keys(objB);

if (keysA.length !== keysB.length) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should comparing key length consider exceptions?

}

// Test for A's keys different from B.
for (let i = 0; i < keysA.length; i++) {
if (exceptions.findIndex(ex => ex === keysA[i]) !== -1) {
continue;
}
if (
!hasOwnProperty.call(objB, keysA[i]) ||
!is(objA[keysA[i]], objB[keysA[i]])
) {
return false;
}
}

return true;
}

/**
* inlined Object.is polyfill to avoid requiring consumers ship their own
* https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is
*/
function is(x, y) {
// SameValue algorithm
if (x === y) {
// Steps 1-5, 7-10
// Steps 6.b-6.e: +0 != -0
// Added the nonzero y check to make Flow happy, but it is redundant
return x !== 0 || y !== 0 || 1 / x === 1 / y;
} else {
// Step 6.a: NaN == NaN
return x !== x && y !== y;
}
}

/**
* Base implementation for the more convenient [`<FlatList>`](/react-native/docs/flatlist.html)
* and [`<SectionList>`](/react-native/docs/sectionlist.html) components, which are also better
Expand Down Expand Up @@ -675,6 +734,12 @@ class VirtualizedList extends React.PureComponent<Props, State> {
if (stickyIndicesFromProps.has(ii + stickyOffset)) {
stickyHeaderIndices.push(cells.length);
}
// remove all components from parentProps so that these components don't cause the CellRenderer to re-render.
const parentProps: Props = {...this.props};
delete parentProps.ItemSeparatorComponent;
delete parentProps.ListEmptyComponent;
delete parentProps.ListFooterComponent;
delete parentProps.ListHeaderComponent;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of copying the whole thing and then removing things you don't want to keep, can you explicitly copy just the props that we do want to keep?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of perf, seems like copy whole thing and set not used property to null will be much better. https://jsperf.com/delete-vs-undefined-vs-null/16

Copy link

@christianbach christianbach Sep 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the original code the only parent props used by CellRenderer are const {renderItem, getItemLayout} = parentProps; and then later !parentProps.debug.
Couldn't we just pass them as regular props and make CellRenderer a PureComponent? The ShallowCompare used by PureComponent would just work and we don't have the need for shallowEqual or deepDiffer. Or perhaps I'm missing something?

cells.push(
<CellRenderer
CellRendererComponent={CellRendererComponent}
Expand All @@ -688,9 +753,9 @@ class VirtualizedList extends React.PureComponent<Props, State> {
key={key}
prevCellKey={prevCellKey}
onUpdateSeparators={this._onUpdateSeparators}
onLayout={e => this._onCellLayout(e, key, ii)}
onLayout={this._onCellLayout}
onUnmount={this._onCellUnmount}
parentProps={this.props}
parentProps={parentProps}
ref={ref => {
this._cellRefs[key] = ref;
}}
Expand Down Expand Up @@ -1046,7 +1111,7 @@ class VirtualizedList extends React.PureComponent<Props, State> {
}
};

_onCellLayout(e, cellKey, index) {
_onCellLayout = (e, cellKey, index): void => {
const layout = e.nativeEvent.layout;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (isNaN(index)) {
  index = 0
}

Maybe you should check index before actually use it.

this._highestMeasuredFrameIndex = Math.max(
         this._highestMeasuredFrameIndex,
         index,	 
       );	

If index is NaN, then _highestMeasuredFrameIndex will be also NaN, which may leads to crash.

const next = {
offset: this._selectOffset(layout),
Expand Down Expand Up @@ -1086,7 +1151,7 @@ class VirtualizedList extends React.PureComponent<Props, State> {
}

this._computeBlankness();
}
};

_onCellUnmount = (cellKey: string) => {
const curr = this._frames[cellKey];
Expand Down Expand Up @@ -1604,7 +1669,7 @@ class CellRenderer extends React.Component<
index: number,
inversionStyle: ?DangerouslyImpreciseStyleProp,
item: Item,
onLayout: (event: Object) => void, // This is extracted by ScrollViewStickyHeader
onLayout: (event: Object, key: string, index: number) => void, // This is extracted by ScrollViewStickyHeader
onUnmount: (cellKey: string) => void,
onUpdateSeparators: (cellKeys: Array<?string>, props: Object) => void,
parentProps: {
Expand Down Expand Up @@ -1670,6 +1735,20 @@ class CellRenderer extends React.Component<
this.props.onUnmount(this.props.cellKey);
}

_onLayout = (e): void =>
this.props.onLayout &&
this.props.onLayout(e, this.props.cellKey, this.props.index);

shouldComponentUpdate(nextProps, nextState) {
if (!shallowEqual(this.props, nextProps, ['parentProps'])) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use deepEquals with a depth limit of 2 instead? That will do a shallow compare of all the props, including parentProps. Hopefully that works and you can get rid of your shallowEqual fork. If that doesn't work for some reason, could you at least rename to shallowEqualWithExceptions and put it in a util folder?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sahrens just to be clear, you are referring to deepDiffer Libraries/Utilities/differ/deepDiffer ?

return true;
}
if (!shallowEqual(this.props.parentProps, nextProps.parentProps)) {
return true;
}
return !shallowEqual(this.state, nextState);
}

render() {
const {
CellRendererComponent,
Expand All @@ -1694,7 +1773,7 @@ class CellRenderer extends React.Component<
* comment and run Flow. */
getItemLayout && !parentProps.debug && !fillRateHelper.enabled()
? undefined
: this.props.onLayout;
: this._onLayout;
// NOTE: that when this is a sticky header, `onLayout` will get automatically extracted and
// called explicitly by `ScrollViewStickyHeader`.
const itemSeparator = ItemSeparatorComponent && (
Expand Down
29 changes: 29 additions & 0 deletions Libraries/Lists/__tests__/VirtualizedList-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,4 +218,33 @@ describe('VirtualizedList', () => {
}),
);
});

it('calls _onCellLayout properly', () => {
const items = [{key: 'i1'}, {key: 'i2'}, {key: 'i3'}];
const mock = jest.fn();
const component = ReactTestRenderer.create(
<VirtualizedList
data={items}
renderItem={({item}) => <item value={item.key} />}
getItem={(data, index) => data[index]}
getItemCount={data => data.length}
/>,
);
const virtualList: VirtualizedList = component.getInstance();
virtualList._onCellLayout = mock;
component.update(
<VirtualizedList
data={[...items, {key: 'i4'}]}
renderItem={({item}) => <item value={item.key} />}
getItem={(data, index) => data[index]}
getItemCount={data => data.length}
/>,
);
const cell = virtualList._cellRefs['i4'];
const event = {
nativeEvent: {layout: {x: 0, y: 0, width: 50, height: 50}},
};
cell._onLayout(event);
expect(mock).toHaveBeenCalledWith(event, 'i4', 3);
});
});