Summary: fix slowness due to layout thrashing when reloading a large … (#12661)

* Summary: fix slowness due to layout thrashing when reloading a large set of status updates

in order to limit the maximum size of a status in a list view (e.g. the home timeline), so as to avoid having to scroll all the way through an abnormally large status update (see https://github.com/tootsuite/mastodon/pull/8205), the following steps are taken:
•the element containing the status is rendered in the browser
•its height is calculated, to determine if it exceeds the maximum height threshold.
Unfortunately for performance, these steps are carried out in the componentDidMount(/Update) method, which also performs style modifications on the element.  The combination of  height request and style modification during javascript evaluation in the browser leads to layout-thrashing, where the elements are repeatedly re-laid-out (see https://developers.google.com/web/fundamentals/performance/rendering/avoid-large-complex-layouts-and-layout-thrashing & https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Performance_best_practices_for_Firefox_fe_engineers).
The solution implemented here is to memoize the collapsed state in Redux the first time the status is seen (e.g. when fetched as part of a small batch, to populate the home timeline) , so that on subsequent re-renders, the value can be queried, rather than recalculated.  This strategy is derived from https://github.com/tootsuite/mastodon/pull/4439 & https://github.com/tootsuite/mastodon/pull/4909, and should resolve https://github.com/tootsuite/mastodon/issues/12455.

Andrew Lin (https://github.com/onethreeseven) is thanked for his assistance in root cause analysis and solution brainstorming

* remove getSnapshotBeforeUpdate from status

* remove componentWillUnmount from status

* persist last-intersected status update and restore when ScrollableList is restored

e.g. when navigating from home-timeline to a status conversational  thread and <Back again

* cache currently-viewing status id to avoid calling redux with identical value

* refactor collapse toggle to pass explicit boolean
This commit is contained in:
Matt Panaro 2019-12-28 23:39:48 -05:00 committed by Eugen Rochko
parent 1155dc0835
commit 31f7c3fc5d
11 changed files with 74 additions and 35 deletions

View file

@ -26,8 +26,9 @@ export const STATUS_UNMUTE_REQUEST = 'STATUS_UNMUTE_REQUEST';
export const STATUS_UNMUTE_SUCCESS = 'STATUS_UNMUTE_SUCCESS'; export const STATUS_UNMUTE_SUCCESS = 'STATUS_UNMUTE_SUCCESS';
export const STATUS_UNMUTE_FAIL = 'STATUS_UNMUTE_FAIL'; export const STATUS_UNMUTE_FAIL = 'STATUS_UNMUTE_FAIL';
export const STATUS_REVEAL = 'STATUS_REVEAL'; export const STATUS_REVEAL = 'STATUS_REVEAL';
export const STATUS_HIDE = 'STATUS_HIDE'; export const STATUS_HIDE = 'STATUS_HIDE';
export const STATUS_COLLAPSE = 'STATUS_COLLAPSE';
export const REDRAFT = 'REDRAFT'; export const REDRAFT = 'REDRAFT';
@ -320,3 +321,11 @@ export function revealStatus(ids) {
ids, ids,
}; };
}; };
export function toggleStatusCollapse(id, isCollapsed) {
return {
type: STATUS_COLLAPSE,
id,
isCollapsed,
};
}

View file

@ -17,6 +17,14 @@ export const TIMELINE_LOAD_PENDING = 'TIMELINE_LOAD_PENDING';
export const TIMELINE_DISCONNECT = 'TIMELINE_DISCONNECT'; export const TIMELINE_DISCONNECT = 'TIMELINE_DISCONNECT';
export const TIMELINE_CONNECT = 'TIMELINE_CONNECT'; export const TIMELINE_CONNECT = 'TIMELINE_CONNECT';
export const CURRENTLY_VIEWING = 'CURRENTLY_VIEWING';
export const updateCurrentlyViewing = (timeline, id) => ({
type: CURRENTLY_VIEWING,
timeline,
id,
});
export const loadPending = timeline => ({ export const loadPending = timeline => ({
type: TIMELINE_LOAD_PENDING, type: TIMELINE_LOAD_PENDING,
timeline, timeline,

View file

@ -20,6 +20,8 @@ export default class IntersectionObserverArticle extends React.Component {
cachedHeight: PropTypes.number, cachedHeight: PropTypes.number,
onHeightChange: PropTypes.func, onHeightChange: PropTypes.func,
children: PropTypes.node, children: PropTypes.node,
currentlyViewing: PropTypes.number,
updateCurrentlyViewing: PropTypes.func,
}; };
state = { state = {
@ -48,6 +50,8 @@ export default class IntersectionObserverArticle extends React.Component {
); );
this.componentMounted = true; this.componentMounted = true;
if(id === this.props.currentlyViewing) this.node.scrollIntoView();
} }
componentWillUnmount () { componentWillUnmount () {
@ -60,6 +64,8 @@ export default class IntersectionObserverArticle extends React.Component {
handleIntersection = (entry) => { handleIntersection = (entry) => {
this.entry = entry; this.entry = entry;
if(entry.intersectionRatio > 0.75 && this.props.updateCurrentlyViewing) this.props.updateCurrentlyViewing(this.id);
scheduleIdleTask(this.calculateHeight); scheduleIdleTask(this.calculateHeight);
this.setState(this.updateStateAfterIntersection); this.setState(this.updateStateAfterIntersection);
} }

View file

@ -36,6 +36,8 @@ export default class ScrollableList extends PureComponent {
emptyMessage: PropTypes.node, emptyMessage: PropTypes.node,
children: PropTypes.node, children: PropTypes.node,
bindToDocument: PropTypes.bool, bindToDocument: PropTypes.bool,
currentlyViewing: PropTypes.number,
updateCurrentlyViewing: PropTypes.func,
}; };
static defaultProps = { static defaultProps = {
@ -309,6 +311,8 @@ export default class ScrollableList extends PureComponent {
listLength={childrenCount} listLength={childrenCount}
intersectionObserverWrapper={this.intersectionObserverWrapper} intersectionObserverWrapper={this.intersectionObserverWrapper}
saveHeightKey={trackScroll ? `${this.context.router.route.location.key}:${scrollKey}` : null} saveHeightKey={trackScroll ? `${this.context.router.route.location.key}:${scrollKey}` : null}
currentlyViewing={this.props.currentlyViewing}
updateCurrentlyViewing={this.props.updateCurrentlyViewing}
> >
{React.cloneElement(child, { {React.cloneElement(child, {
getScrollPosition: this.getScrollPosition, getScrollPosition: this.getScrollPosition,

View file

@ -76,6 +76,7 @@ class Status extends ImmutablePureComponent {
onEmbed: PropTypes.func, onEmbed: PropTypes.func,
onHeightChange: PropTypes.func, onHeightChange: PropTypes.func,
onToggleHidden: PropTypes.func, onToggleHidden: PropTypes.func,
onToggleCollapsed: PropTypes.func,
muted: PropTypes.bool, muted: PropTypes.bool,
hidden: PropTypes.bool, hidden: PropTypes.bool,
unread: PropTypes.bool, unread: PropTypes.bool,
@ -107,14 +108,6 @@ class Status extends ImmutablePureComponent {
this.didShowCard = !this.props.muted && !this.props.hidden && this.props.status && this.props.status.get('card'); this.didShowCard = !this.props.muted && !this.props.hidden && this.props.status && this.props.status.get('card');
} }
getSnapshotBeforeUpdate () {
if (this.props.getScrollPosition) {
return this.props.getScrollPosition();
} else {
return null;
}
}
static getDerivedStateFromProps(nextProps, prevState) { static getDerivedStateFromProps(nextProps, prevState) {
if (nextProps.status && nextProps.status.get('id') !== prevState.statusId) { if (nextProps.status && nextProps.status.get('id') !== prevState.statusId) {
return { return {
@ -141,17 +134,6 @@ class Status extends ImmutablePureComponent {
} }
} }
componentWillUnmount() {
if (this.node && this.props.getScrollPosition) {
const position = this.props.getScrollPosition();
if (position !== null && this.node.offsetTop < position.top) {
requestAnimationFrame(() => {
this.props.updateScrollBottom(position.height - position.top);
});
}
}
}
handleToggleMediaVisibility = () => { handleToggleMediaVisibility = () => {
this.setState({ showMedia: !this.state.showMedia }); this.setState({ showMedia: !this.state.showMedia });
} }
@ -196,7 +178,11 @@ class Status extends ImmutablePureComponent {
handleExpandedToggle = () => { handleExpandedToggle = () => {
this.props.onToggleHidden(this._properStatus()); this.props.onToggleHidden(this._properStatus());
}; }
handleCollapsedToggle = isCollapsed => {
this.props.onToggleCollapsed(this._properStatus(), isCollapsed);
}
renderLoadingMediaGallery () { renderLoadingMediaGallery () {
return <div className='media-gallery' style={{ height: '110px' }} />; return <div className='media-gallery' style={{ height: '110px' }} />;
@ -466,7 +452,7 @@ class Status extends ImmutablePureComponent {
</a> </a>
</div> </div>
<StatusContent status={status} onClick={this.handleClick} expanded={!status.get('hidden')} onExpandedToggle={this.handleExpandedToggle} collapsable /> <StatusContent status={status} onClick={this.handleClick} expanded={!status.get('hidden')} onExpandedToggle={this.handleExpandedToggle} collapsable onCollapsedToggle={this.handleCollapsedToggle} />
{media} {media}

View file

@ -23,11 +23,11 @@ export default class StatusContent extends React.PureComponent {
onExpandedToggle: PropTypes.func, onExpandedToggle: PropTypes.func,
onClick: PropTypes.func, onClick: PropTypes.func,
collapsable: PropTypes.bool, collapsable: PropTypes.bool,
onCollapsedToggle: PropTypes.func,
}; };
state = { state = {
hidden: true, hidden: true,
collapsed: null, // `collapsed: null` indicates that an element doesn't need collapsing, while `true` or `false` indicates that it does (and is/isn't).
}; };
_updateStatusLinks () { _updateStatusLinks () {
@ -62,14 +62,16 @@ export default class StatusContent extends React.PureComponent {
link.setAttribute('rel', 'noopener noreferrer'); link.setAttribute('rel', 'noopener noreferrer');
} }
if ( if (this.props.status.get('collapsed', null) === null) {
this.props.collapsable let collapsed =
&& this.props.onClick this.props.collapsable
&& this.state.collapsed === null && this.props.onClick
&& node.clientHeight > MAX_HEIGHT && node.clientHeight > MAX_HEIGHT
&& this.props.status.get('spoiler_text').length === 0 && this.props.status.get('spoiler_text').length === 0;
) {
this.setState({ collapsed: true }); if(this.props.onCollapsedToggle) this.props.onCollapsedToggle(collapsed);
this.props.status.set('collapsed', collapsed);
} }
} }
@ -178,6 +180,7 @@ export default class StatusContent extends React.PureComponent {
} }
const hidden = this.props.onExpandedToggle ? !this.props.expanded : this.state.hidden; const hidden = this.props.onExpandedToggle ? !this.props.expanded : this.state.hidden;
const renderReadMore = this.props.onClick && status.get('collapsed');
const content = { __html: status.get('contentHtml') }; const content = { __html: status.get('contentHtml') };
const spoilerContent = { __html: status.get('spoilerHtml') }; const spoilerContent = { __html: status.get('spoilerHtml') };
@ -185,7 +188,7 @@ export default class StatusContent extends React.PureComponent {
const classNames = classnames('status__content', { const classNames = classnames('status__content', {
'status__content--with-action': this.props.onClick && this.context.router, 'status__content--with-action': this.props.onClick && this.context.router,
'status__content--with-spoiler': status.get('spoiler_text').length > 0, 'status__content--with-spoiler': status.get('spoiler_text').length > 0,
'status__content--collapsed': this.state.collapsed === true, 'status__content--collapsed': renderReadMore,
}); });
if (isRtl(status.get('search_index'))) { if (isRtl(status.get('search_index'))) {
@ -237,7 +240,7 @@ export default class StatusContent extends React.PureComponent {
</div>, </div>,
]; ];
if (this.state.collapsed) { if (renderReadMore) {
output.push(readMoreButton); output.push(readMoreButton);
} }

View file

@ -26,6 +26,8 @@ export default class StatusList extends ImmutablePureComponent {
emptyMessage: PropTypes.node, emptyMessage: PropTypes.node,
alwaysPrepend: PropTypes.bool, alwaysPrepend: PropTypes.bool,
timelineId: PropTypes.string, timelineId: PropTypes.string,
currentlyViewing: PropTypes.number,
updateCurrentlyViewing: PropTypes.func,
}; };
static defaultProps = { static defaultProps = {
@ -58,6 +60,12 @@ export default class StatusList extends ImmutablePureComponent {
this.props.onLoadMore(this.props.statusIds.size > 0 ? this.props.statusIds.last() : undefined); this.props.onLoadMore(this.props.statusIds.size > 0 ? this.props.statusIds.last() : undefined);
}, 300, { leading: true }) }, 300, { leading: true })
updateCurrentlyViewingWithCache = (id) => {
if(this.cachedCurrentlyViewing === id) return;
this.cachedCurrentlyViewing = id;
this.props.updateCurrentlyViewing(id);
}
_selectChild (index, align_top) { _selectChild (index, align_top) {
const container = this.node.node; const container = this.node.node;
const element = container.querySelector(`article:nth-of-type(${index + 1}) .focusable`); const element = container.querySelector(`article:nth-of-type(${index + 1}) .focusable`);
@ -79,6 +87,7 @@ export default class StatusList extends ImmutablePureComponent {
render () { render () {
const { statusIds, featuredStatusIds, shouldUpdateScroll, onLoadMore, timelineId, ...other } = this.props; const { statusIds, featuredStatusIds, shouldUpdateScroll, onLoadMore, timelineId, ...other } = this.props;
const { isLoading, isPartial } = other; const { isLoading, isPartial } = other;
other.updateCurrentlyViewing = this.updateCurrentlyViewingWithCache;
if (isPartial) { if (isPartial) {
return <RegenerationIndicator />; return <RegenerationIndicator />;

View file

@ -23,6 +23,7 @@ import {
deleteStatus, deleteStatus,
hideStatus, hideStatus,
revealStatus, revealStatus,
toggleStatusCollapse,
} from '../actions/statuses'; } from '../actions/statuses';
import { import {
unmuteAccount, unmuteAccount,
@ -190,6 +191,10 @@ const mapDispatchToProps = (dispatch, { intl }) => ({
} }
}, },
onToggleCollapsed (status, isCollapsed) {
dispatch(toggleStatusCollapse(status.get('id'), isCollapsed));
},
onBlockDomain (domain) { onBlockDomain (domain) {
dispatch(openModal('CONFIRM', { dispatch(openModal('CONFIRM', {
message: <FormattedMessage id='confirmations.domain_block.message' defaultMessage='Are you really, really sure you want to block the entire {domain}? In most cases a few targeted blocks or mutes are sufficient and preferable. You will not see content from that domain in any public timelines or your notifications. Your followers from that domain will be removed.' values={{ domain: <strong>{domain}</strong> }} />, message: <FormattedMessage id='confirmations.domain_block.message' defaultMessage='Are you really, really sure you want to block the entire {domain}? In most cases a few targeted blocks or mutes are sufficient and preferable. You will not see content from that domain in any public timelines or your notifications. Your followers from that domain will be removed.' values={{ domain: <strong>{domain}</strong> }} />,

View file

@ -1,6 +1,6 @@
import { connect } from 'react-redux'; import { connect } from 'react-redux';
import StatusList from '../../../components/status_list'; import StatusList from '../../../components/status_list';
import { scrollTopTimeline, loadPending } from '../../../actions/timelines'; import { scrollTopTimeline, loadPending, updateCurrentlyViewing } from '../../../actions/timelines';
import { Map as ImmutableMap, List as ImmutableList } from 'immutable'; import { Map as ImmutableMap, List as ImmutableList } from 'immutable';
import { createSelector } from 'reselect'; import { createSelector } from 'reselect';
import { debounce } from 'lodash'; import { debounce } from 'lodash';
@ -39,6 +39,7 @@ const makeMapStateToProps = () => {
isPartial: state.getIn(['timelines', timelineId, 'isPartial'], false), isPartial: state.getIn(['timelines', timelineId, 'isPartial'], false),
hasMore: state.getIn(['timelines', timelineId, 'hasMore']), hasMore: state.getIn(['timelines', timelineId, 'hasMore']),
numPending: getPendingStatusIds(state, { type: timelineId }).size, numPending: getPendingStatusIds(state, { type: timelineId }).size,
currentlyViewing: state.getIn(['timelines', timelineId, 'currentlyViewing'], -1),
}); });
return mapStateToProps; return mapStateToProps;
@ -56,6 +57,7 @@ const mapDispatchToProps = (dispatch, { timelineId }) => ({
onLoadPending: () => dispatch(loadPending(timelineId)), onLoadPending: () => dispatch(loadPending(timelineId)),
updateCurrentlyViewing: id => dispatch(updateCurrentlyViewing(timelineId, id)),
}); });
export default connect(makeMapStateToProps, mapDispatchToProps)(StatusList); export default connect(makeMapStateToProps, mapDispatchToProps)(StatusList);

View file

@ -12,6 +12,7 @@ import {
STATUS_UNMUTE_SUCCESS, STATUS_UNMUTE_SUCCESS,
STATUS_REVEAL, STATUS_REVEAL,
STATUS_HIDE, STATUS_HIDE,
STATUS_COLLAPSE,
} from '../actions/statuses'; } from '../actions/statuses';
import { TIMELINE_DELETE } from '../actions/timelines'; import { TIMELINE_DELETE } from '../actions/timelines';
import { STATUS_IMPORT, STATUSES_IMPORT } from '../actions/importer'; import { STATUS_IMPORT, STATUSES_IMPORT } from '../actions/importer';
@ -73,6 +74,8 @@ export default function statuses(state = initialState, action) {
} }
}); });
}); });
case STATUS_COLLAPSE:
return state.setIn([action.id, 'collapsed'], action.isCollapsed);
case TIMELINE_DELETE: case TIMELINE_DELETE:
return deleteStatus(state, action.id, action.references); return deleteStatus(state, action.id, action.references);
default: default:

View file

@ -9,6 +9,7 @@ import {
TIMELINE_CONNECT, TIMELINE_CONNECT,
TIMELINE_DISCONNECT, TIMELINE_DISCONNECT,
TIMELINE_LOAD_PENDING, TIMELINE_LOAD_PENDING,
CURRENTLY_VIEWING,
} from '../actions/timelines'; } from '../actions/timelines';
import { import {
ACCOUNT_BLOCK_SUCCESS, ACCOUNT_BLOCK_SUCCESS,
@ -28,6 +29,7 @@ const initialTimeline = ImmutableMap({
hasMore: true, hasMore: true,
pendingItems: ImmutableList(), pendingItems: ImmutableList(),
items: ImmutableList(), items: ImmutableList(),
currentlyViewing: -1,
}); });
const expandNormalizedTimeline = (state, timeline, statuses, next, isPartial, isLoadingRecent, usePendingItems) => { const expandNormalizedTimeline = (state, timeline, statuses, next, isPartial, isLoadingRecent, usePendingItems) => {
@ -168,6 +170,8 @@ export default function timelines(state = initialState, action) {
initialTimeline, initialTimeline,
map => map.set('online', false).update(action.usePendingItems ? 'pendingItems' : 'items', items => items.first() ? items.unshift(null) : items) map => map.set('online', false).update(action.usePendingItems ? 'pendingItems' : 'items', items => items.first() ? items.unshift(null) : items)
); );
case CURRENTLY_VIEWING:
return state.update(action.timeline, initialTimeline, map => map.set('currentlyViewing', action.id));
default: default:
return state; return state;
} }