From fbeda25e7e87e2067a214f1580cab87b094fc71a Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 11 Dec 2019 15:28:21 -0800 Subject: [PATCH 1/5] Reduce wasted render for activities --- CHANGELOG.md | 1 + packages/component/src/BasicTranscript.js | 58 ++++++++++--------- .../src/hooks/internal/useMemoArrayMap.js | 32 ++++++++++ 3 files changed, 65 insertions(+), 26 deletions(-) create mode 100644 packages/component/src/hooks/internal/useMemoArrayMap.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 058985e432..6cd2270ee6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -76,6 +76,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. - Moved `` from `` to `` - Moved WebSpeechPonyfill patching code from `` to `` - Fixes [#2699](https://github.com/microsoft/BotFramework-WebChat/issues/2699). Disable speech recognition and synthesis when using Direct Line Speech under IE11, in PR [#2649](https://github.com/microsoft/BotFramework-WebChat/pull/2649) +- Fixes [#2709](https://github.com/microsoft/BotFramework-WebChat/issues/2709). Reduce wasted render of activities by memoizing partial result of ``, in PR [#2710](https://github.com/microsoft/BotFramework-WebChat/pull/2710) ### Changed diff --git a/packages/component/src/BasicTranscript.js b/packages/component/src/BasicTranscript.js index c5b86ab88c..f6f9925ccf 100644 --- a/packages/component/src/BasicTranscript.js +++ b/packages/component/src/BasicTranscript.js @@ -2,7 +2,7 @@ import { css } from 'glamor'; import { Panel as ScrollToBottomPanel } from 'react-scroll-to-bottom'; import classNames from 'classnames'; import PropTypes from 'prop-types'; -import React, { useMemo } from 'react'; +import React, { useCallback, useMemo } from 'react'; import ScrollToEndButton from './Activity/ScrollToEndButton'; import SpeakActivity from './Activity/Speak'; @@ -12,6 +12,8 @@ import useRenderActivity from './hooks/useRenderActivity'; import useRenderAttachment from './hooks/useRenderAttachment'; import useStyleOptions from './hooks/useStyleOptions'; import useStyleSet from './hooks/useStyleSet'; +import useTrackChanges from './hooks/internal/useTrackChanges'; +import useMemoArrayMap from './hooks/internal/useMemoArrayMap'; const ROOT_CSS = css({ overflow: 'hidden', @@ -62,44 +64,48 @@ const BasicTranscript = ({ className }) => { const [groupTimestamp] = useGroupTimestamp(); const renderAttachment = useRenderAttachment(); const renderActivity = useRenderActivity(renderAttachment); + const renderActivityElement = useCallback( + activity => { + const element = renderActivity({ + activity, + timestampClassName: 'transcript-timestamp' + }); + + return element && { activity, element }; + }, + [renderActivity] + ); // We use 2-pass approach for rendering activities, for show/hide timestamp grouping. // Until the activity pass thru middleware, we never know if it is going to show up. // After we know which activities will show up, we can compute which activity will show timestamps. // If the activity does not render, it will not be spoken if text-to-speech is enabled. - const activityElements = useMemo( - () => - activities.reduce((activityElements, activity) => { - const element = renderActivity({ - activity, - timestampClassName: 'transcript-timestamp' - }); - element && activityElements.push({ activity, element }); - - return activityElements; - }, []), - [activities, renderActivity] - ); + const activityElements = useMemoArrayMap(activities, renderActivityElement); + // TODO: [P2] We can also use useMemoArrayMap() for this function. + // useMemoArrayMap(array, mapper) will need to be modified to useMemoArrayMap(array, mapper, getDeps). + // This is because the deps for every item is not itself anymore. It will include activityElements[index + 1]. const activityElementsWithMetadata = useMemo( () => - activityElements.map((activityElement, index) => { - const { activity } = activityElement; - const { activity: nextActivity } = activityElements[index + 1] || {}; + activityElements + .filter(activityElement => activityElement) + .map((activityElement, index) => { + const { activity } = activityElement; + const { activity: nextActivity } = activityElements[index + 1] || {}; - return { - ...activityElement, + return { + ...activityElement, - key: (activity.channelData && activity.channelData.clientActivityID) || activity.id || index, + key: (activity.channelData && activity.channelData.clientActivityID) || activity.id || index, - // TODO: [P2] We should use core/definitions/speakingActivity for this predicate instead - shouldSpeak: activity.channelData && activity.channelData.speak, + // TODO: [P2] We should use core/definitions/speakingActivity for this predicate instead + shouldSpeak: activity.channelData && activity.channelData.speak, - // Hide timestamp if same timestamp group with the next activity - timestampVisible: !sameTimestampGroup(activity, nextActivity, groupTimestamp) - }; - }), + // Hide timestamp if same timestamp group with the next activity + timestampVisible: !sameTimestampGroup(activity, nextActivity, groupTimestamp) + }; + }), [activityElements, groupTimestamp] ); diff --git a/packages/component/src/hooks/internal/useMemoArrayMap.js b/packages/component/src/hooks/internal/useMemoArrayMap.js new file mode 100644 index 0000000000..dd0020202e --- /dev/null +++ b/packages/component/src/hooks/internal/useMemoArrayMap.js @@ -0,0 +1,32 @@ +import { useMemo, useRef } from 'react'; + +export default function useMemoArrayMap(array, mapper) { + const prevMapperRef = useRef(); + const sameMapper = Object.is(mapper, prevMapperRef.current); + + const prevMapperCallsRef = useRef([]); + const { current: prevMapperCalls = {} } = sameMapper ? prevMapperCallsRef : {}; + const nextMapperCalls = []; + + return useMemo(() => { + const mapped = array.map((value, index) => { + const prevResult = prevMapperCalls.find(({ value: targetValue }) => targetValue === value); + let result; + + if (prevResult) { + result = prevResult.result; + } else { + result = mapper.call(array, value, index); + } + + nextMapperCalls.push({ result, value }); + + return result; + }); + + prevMapperCallsRef.current = nextMapperCalls; + prevMapperRef.current = mapper; + + return mapped; + }, [array, mapper]); +} From a0035f325fec03a672ef7b572b727bcc0fd58e27 Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 11 Dec 2019 15:32:10 -0800 Subject: [PATCH 2/5] Fix ESLint --- packages/component/src/BasicTranscript.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/component/src/BasicTranscript.js b/packages/component/src/BasicTranscript.js index f6f9925ccf..dedf5fc0d8 100644 --- a/packages/component/src/BasicTranscript.js +++ b/packages/component/src/BasicTranscript.js @@ -12,7 +12,6 @@ import useRenderActivity from './hooks/useRenderActivity'; import useRenderAttachment from './hooks/useRenderAttachment'; import useStyleOptions from './hooks/useStyleOptions'; import useStyleSet from './hooks/useStyleSet'; -import useTrackChanges from './hooks/internal/useTrackChanges'; import useMemoArrayMap from './hooks/internal/useMemoArrayMap'; const ROOT_CSS = css({ From 4ce32095df476a6fb352da97da1c9b1709a17c5b Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 11 Dec 2019 15:35:18 -0800 Subject: [PATCH 3/5] Fix ESLint --- packages/component/src/hooks/internal/useMemoArrayMap.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/component/src/hooks/internal/useMemoArrayMap.js b/packages/component/src/hooks/internal/useMemoArrayMap.js index dd0020202e..3525dbc1e9 100644 --- a/packages/component/src/hooks/internal/useMemoArrayMap.js +++ b/packages/component/src/hooks/internal/useMemoArrayMap.js @@ -11,13 +11,7 @@ export default function useMemoArrayMap(array, mapper) { return useMemo(() => { const mapped = array.map((value, index) => { const prevResult = prevMapperCalls.find(({ value: targetValue }) => targetValue === value); - let result; - - if (prevResult) { - result = prevResult.result; - } else { - result = mapper.call(array, value, index); - } + const { result = mapper.call(array, value, index) } = prevResult || {}; nextMapperCalls.push({ result, value }); From 359e684debc36c973e2e93f82fb0889f080f8ada Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 11 Dec 2019 16:01:01 -0800 Subject: [PATCH 4/5] Fix tests --- packages/component/src/BasicTranscript.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/component/src/BasicTranscript.js b/packages/component/src/BasicTranscript.js index dedf5fc0d8..f597d02fc7 100644 --- a/packages/component/src/BasicTranscript.js +++ b/packages/component/src/BasicTranscript.js @@ -85,13 +85,14 @@ const BasicTranscript = ({ className }) => { // TODO: [P2] We can also use useMemoArrayMap() for this function. // useMemoArrayMap(array, mapper) will need to be modified to useMemoArrayMap(array, mapper, getDeps). // This is because the deps for every item is not itself anymore. It will include activityElements[index + 1]. + const trimmedActivityElements = activityElements.filter(activityElement => activityElement); const activityElementsWithMetadata = useMemo( () => - activityElements + trimmedActivityElements .filter(activityElement => activityElement) .map((activityElement, index) => { const { activity } = activityElement; - const { activity: nextActivity } = activityElements[index + 1] || {}; + const { activity: nextActivity } = trimmedActivityElements[index + 1] || {}; return { ...activityElement, @@ -105,7 +106,7 @@ const BasicTranscript = ({ className }) => { timestampVisible: !sameTimestampGroup(activity, nextActivity, groupTimestamp) }; }), - [activityElements, groupTimestamp] + [groupTimestamp, trimmedActivityElements] ); return ( From 8a6502606c12bfdd5cb24f5e016d2b179d99d81e Mon Sep 17 00:00:00 2001 From: William Wong Date: Wed, 11 Dec 2019 17:18:39 -0800 Subject: [PATCH 5/5] Fix test --- packages/component/src/hooks/internal/useMemoArrayMap.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/component/src/hooks/internal/useMemoArrayMap.js b/packages/component/src/hooks/internal/useMemoArrayMap.js index 3525dbc1e9..1e69d1bb66 100644 --- a/packages/component/src/hooks/internal/useMemoArrayMap.js +++ b/packages/component/src/hooks/internal/useMemoArrayMap.js @@ -5,7 +5,7 @@ export default function useMemoArrayMap(array, mapper) { const sameMapper = Object.is(mapper, prevMapperRef.current); const prevMapperCallsRef = useRef([]); - const { current: prevMapperCalls = {} } = sameMapper ? prevMapperCallsRef : {}; + const { current: prevMapperCalls = [] } = sameMapper ? prevMapperCallsRef : {}; const nextMapperCalls = []; return useMemo(() => {