For-each loop optimization and memoize normalizeKey #3469
Conversation
src/utils/stackingUtils.ts
Outdated
| @@ -173,6 +183,6 @@ export function stackedExtent(stackingResult: StackingResult, keyAccessor: IAcce | |||
| * @param {any} key The key to be normalized | |||
| * @return {string} The stringified key | |||
| */ | |||
| export function normalizeKey(key: any) { | |||
| export const normalizeKey = lMemoize((key: any) => { | |||
There was a problem hiding this comment.
@themadcreator I believe that any key passed in here should be immutable, but given it's typed as any, would want a sanity check?
There was a problem hiding this comment.
I'm not sure this memoization will actually help because in order to create the cache key and store the value, it will convert the argument into a Map key which might be just as slow as the String(key) below
src/utils/stackingUtils.ts
Outdated
| @@ -173,6 +183,6 @@ export function stackedExtent(stackingResult: StackingResult, keyAccessor: IAcce | |||
| * @param {any} key The key to be normalized | |||
| * @return {string} The stringified key | |||
| */ | |||
| export function normalizeKey(key: any) { | |||
| export const normalizeKey = lMemoize((key: any) => { | |||
There was a problem hiding this comment.
I'm not sure this memoization will actually help because in order to create the cache key and store the value, it will convert the argument into a Map key which might be just as slow as the String(key) below
src/utils/stackingUtils.ts
Outdated
| @@ -11,6 +11,8 @@ import { IAccessor } from "../core/interfaces"; | |||
| import * as Utils from "./"; | |||
| import { makeEnum } from "./makeEnum"; | |||
|
|
|||
| const lMemoize = require("lodash.memoize"); | |||
There was a problem hiding this comment.
even though this keeps the library size smaller, i think i'd prefer to use the normal import syntax. Unfortunately the modular lodash packages don't support import statements for some reason.
There was a problem hiding this comment.
There are some instructions on how to get treeshaking to work with lodash, but i couldn't get it to work
https://medium.com/@martin_hotell/tree-shake-lodash-with-webpack-jest-and-typescript-2734fa13b5cd
src/drawers/rectangleDrawer.ts
Outdated
| data.forEach((datum, index) => { | ||
| const dataLen = data.length; | ||
| for (let index = 0; index < dataLen; index++ ) { | ||
| const datum = data[index]; | ||
| if (datum == null) { | ||
| return; |
src/plots/piePlot.ts
Outdated
| const dataLen = data.length; | ||
| for (let datumIndex = 0; datumIndex < dataLen; datumIndex++) { | ||
| const datum = data[datumIndex]; | ||
|
|
||
| let value = this.sectorValue().accessor(datum, datumIndex, dataset); | ||
| if (!Utils.Math.isValidNumber(value)) { | ||
| return; |
src/plots/plot.ts
Outdated
| const data = dataset.data(); | ||
| const dataLen = data.length; | ||
| for (let datumIndex = 0; datumIndex < dataLen; datumIndex++) { | ||
| const datum = data[datumIndex]; | ||
| const position = this._pixelPoint(datum, datumIndex, dataset); | ||
| if (Utils.Math.isNaN(position.x) || Utils.Math.isNaN(position.y)) { | ||
| return; |
| const dataLength = data.length; | ||
| for (let datumIndex = 0; datumIndex < dataLength; datumIndex++) { | ||
| const datum = data[datumIndex]; | ||
|
|
||
| if (datum == null) { |
There was a problem hiding this comment.
(addressed, but diff doesn't recognize)
src/plots/scatterPlot.ts
Outdated
| const data = dataToDraw.get(dataset); | ||
| const dataLen = data.length; | ||
| for (let index = 0; index < dataLen; index++) { | ||
| const datum = data[index]; | ||
| if (datum == null) { | ||
| return; |
src/utils/stackingUtils.ts
Outdated
| const data = dataset.data(); | ||
| const dataLen = data.length; | ||
| for (let index = 0; index < dataLen; index++) { | ||
| const datum = data[index]; | ||
| if (filter != null && !filter(datum, index, dataset)) { | ||
| return; |
Fix line plot indicesDemo: quicktests | fiddle |
Optimization around for-each loops (https://jsperf.com/fast-array-foreach ) for a comparison.
Memoization of
normalizeKeyfunction, would like a sanity check to make sure thatnormalizeKeynever gets called with a mutable object.Note: the memoization here isn't necessarily a runtime optimization, but a garbage collection optimization/trade-off
Was seeing excessive GC (especially by Firefox) when running without the memoized keys in production environment