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

For-each loop optimization and memoize normalizeKey #3469

Merged
merged 5 commits into from
May 31, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
"d3-ease": "^1.0.0",
"d3-shape": "^1.0.0",
"is-plain-object": "^2.0.4",
"lodash.memoize": "^4.1.2",
"tslib": "~1.8.0",
"typesettable": "4.1.0"
}
Expand Down
6 changes: 4 additions & 2 deletions src/drawers/rectangleDrawer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,17 @@ export const RectangleCanvasDrawStep: CanvasDrawStep = (
data: any[],
projector: AttributeToAppliedProjector) => {
context.save();
data.forEach((datum, index) => {
const dataLen = data.length;
for (let index = 0; index < dataLen; index++ ) {
const datum = data[index];
if (datum == null) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

return -> continue

}
const attrs = resolveAttributesSubsetWithStyles(projector, RECT_ATTRS, datum, index);
context.beginPath();
context.rect(attrs["x"], attrs["y"], attrs["width"], attrs["height"]);
renderPathWithStyle(context, attrs);
});
}
context.restore();
};

Expand Down
6 changes: 4 additions & 2 deletions src/drawers/svgDrawer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,12 @@ export class SVGDrawer implements IDrawer {
this._createAndDestroyDOMElements(data);

let delay = 0;
appliedDrawSteps.forEach((drawStep, i) => {
const drawLength = appliedDrawSteps.length;
for (let i = 0; i < drawLength; i++) {
const drawStep = appliedDrawSteps[i];
Utils.Window.setTimeout(() => this._drawStep(drawStep), delay);
delay += drawStep.animator.totalTime(data.length);
});
}
}

public getVisualPrimitives() {
Expand Down
7 changes: 5 additions & 2 deletions src/plots/barPlot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -447,11 +447,14 @@ export class Bar<X, Y> extends XYPlot<X, Y> {

private _entitiesIntersecting(xValOrRange: number | Range, yValOrRange: number | Range): IPlotEntity[] {
const intersected: IPlotEntity[] = [];
this._getEntityStore().entities().forEach((entity) => {
const entities = this._getEntityStore().entities();
const entitiesLen = entities.length;
for (let i = 0; i < entitiesLen; i++) {
const entity = entities[i];
if (Utils.DOM.intersectsBBox(xValOrRange, yValOrRange, this._entityBounds(entity))) {
intersected.push(this._lightweightPlotEntityToPlotEntity(entity));
}
});
}
return intersected;
}

Expand Down
28 changes: 22 additions & 6 deletions src/plots/linePlot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -444,9 +444,12 @@ export class Line<X> extends XYPlot<X, number> {
let closest: IPlotEntity;

const chartBounds = this.bounds();
this.entities().forEach((entity) => {
const entities = this.entities();
const entityLen = entities.length;
for (let i = 0; i < entityLen; i++) {
const entity = entities[i];
if (!Utils.Math.within(entity.position, chartBounds)) {
return;
continue;
}
const xDist = Math.abs(queryPoint.x - entity.position.x);
const yDist = Math.abs(queryPoint.y - entity.position.y);
Expand All @@ -456,7 +459,7 @@ export class Line<X> extends XYPlot<X, number> {
minXDist = xDist;
minYDist = yDist;
}
});
}

return closest;
}
Expand Down Expand Up @@ -530,7 +533,12 @@ export class Line<X> extends XYPlot<X, number> {
return;
}

let filteredDataIndices = data.map((d, i) => i);
let filteredDataIndices = [];
const dataLen = data.length;
for (let i = 0; i < dataLen; i++) {
filteredDataIndices[i] = i;
}

if (this._croppedRenderingEnabled) {
filteredDataIndices = this._filterCroppedRendering(dataset, filteredDataIndices);
}
Expand All @@ -540,7 +548,14 @@ export class Line<X> extends XYPlot<X, number> {
if (this._collapseDenseVerticalLinesEnabled) {
filteredDataIndices = this._filterDenseLines(dataset, filteredDataIndices);
}
dataToDraw.set(dataset, [filteredDataIndices.map((d, i) => data[d])]);

const filteredData = [];
const filteredIndicesLen = filteredDataIndices.length;
for (let i = 0; i < filteredIndicesLen; i++) {
filteredData[i] = data[i];
}

dataToDraw.set(dataset, [filteredData]);
});

return dataToDraw;
Expand Down Expand Up @@ -690,7 +705,8 @@ export class Line<X> extends XYPlot<X, number> {
const data = dataset.data();
let bucket: Utils.Bucket = null;

for (let ii = 0; ii <= indices.length; ++ii) {
const indicesLength = indices.length;
for (let ii = 0; ii <= indicesLength; ++ii) {
const i = indices[ii];
if (data[i] == null) {
continue;
Expand Down
7 changes: 5 additions & 2 deletions src/plots/piePlot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,10 @@ export class Pie extends Plot {
const writer = new Typesettable.Writer(measurer, context);
const dataset = this.datasets()[0];
const data = this._getDataToDraw().get(dataset);
data.forEach((datum, datumIndex) => {
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

return -> continue

Expand Down Expand Up @@ -647,6 +650,6 @@ export class Pie extends Plot {
xAlign: "center",
yAlign: "center",
}, g.node());
});
}
}
}
7 changes: 5 additions & 2 deletions src/plots/plot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,10 @@ export class Plot extends Component {
const drawer = this._datasetToDrawer.get(dataset);
let validDatumIndex = 0;

dataset.data().forEach((datum: any, datumIndex: number) => {
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

return -> continue

Expand All @@ -724,7 +727,7 @@ export class Plot extends Component {
validDatumIndex,
});
validDatumIndex++;
});
}
});

return lightweightPlotEntities;
Expand Down
10 changes: 7 additions & 3 deletions src/plots/rectanglePlot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,10 @@ export class Rectangle<X, Y> extends XYPlot<X, Y> {
const yMin = Math.min.apply(null, yRange);
const yMax = Math.max.apply(null, yRange);
const data = dataToDraw.get(dataset);
data.forEach((datum, datumIndex) => {
const dataLength = data.length;
for (let datumIndex = 0; datumIndex < dataLength; datumIndex++) {
const datum = data[datumIndex];

if (datum == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

return -> continue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(addressed, but diff doesn't recognize)

return;
}
Expand Down Expand Up @@ -447,7 +450,7 @@ export class Rectangle<X, Y> extends XYPlot<X, Y> {
yAlign: "center",
}, g.node());
}
});
}
}

private _overlayLabel(labelXRange: Range, labelYRange: Range, datumIndex: number, datasetIndex: number,
Expand All @@ -457,7 +460,8 @@ export class Rectangle<X, Y> extends XYPlot<X, Y> {
for (let i = datasetIndex; i < datasets.length; i++) {
const dataset = datasets[i];
const data = dataToDraw.get(dataset);
for (let j = (i === datasetIndex ? datumIndex + 1 : 0); j < data.length; j++) {
const dataLen = data.length;
for (let j = (i === datasetIndex ? datumIndex + 1 : 0); j < dataLen; j++) {
if (Utils.DOM.intersectsBBox(labelXRange, labelYRange, this._entityBBox(data[j], j, dataset, attrToProjector))) {
return true;
}
Expand Down
7 changes: 5 additions & 2 deletions src/plots/scatterPlot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,12 +277,15 @@ export class Scatter<X, Y> extends XYPlot<X, Y> {
const dataToDraw = this._getDataToDraw();
const attrToProjector = this._getAttrToProjector();
this.datasets().forEach((dataset) => {
dataToDraw.get(dataset).forEach((datum, index) => {
const data = dataToDraw.get(dataset);
const dataLen = data.length;
for (let index = 0; index < dataLen; index++) {
const datum = data[index];
if (datum == null) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

return -> continue

}
this._drawLabel(datum, index, dataset, attrToProjector);
});
}
});
}

Expand Down
7 changes: 5 additions & 2 deletions src/plots/segmentPlot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -224,11 +224,14 @@ export class Segment<X, Y> extends XYPlot<X, Y> {
private _entitiesIntersecting(xRange: Range, yRange: Range): IPlotEntity[] {
const intersected: IPlotEntity[] = [];
const attrToProjector = this._getAttrToProjector();
this.entities().forEach((entity) => {
const entities = this.entities();
const entitiesLen = entities.length;
for (let i = 0; i < entitiesLen; i++) {
const entity = entities[i];
if (this._lineIntersectsBox(entity, xRange, yRange, attrToProjector)) {
intersected.push(entity);
}
});
}
return intersected;
}

Expand Down
7 changes: 5 additions & 2 deletions src/plots/stackedAreaPlot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -248,9 +248,12 @@ export class StackedArea<X> extends Area<X> {
private static _domainKeys(datasets: Dataset[], keyAccessor: IAccessor<any>) {
const domainKeys = d3.set();
datasets.forEach((dataset) => {
dataset.data().forEach((datum, index) => {
const data = dataset.data();
const dataLen = data.length;
for (let index = 0; index < dataLen; index++) {
const datum = data[index];
domainKeys.add(keyAccessor(datum, index, dataset));
});
}
});

return domainKeys.values();
Expand Down
7 changes: 5 additions & 2 deletions src/plots/waterfallPlot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,10 @@ export class Waterfall<X, Y> extends Bar<X, number> {
let max = Number.MIN_VALUE;
let total = 0;
let hasStarted = false;
dataset.data().forEach((datum, index) => {
const data = dataset.data();
const dataLen = data.length;
for (let index = 0; index < dataLen; index ++) {
const datum = data[index];
const currentValue = this.y().accessor(datum, index, dataset);
const isTotal = this.total().accessor(datum, index, dataset);
if (!isTotal || index === 0) {
Expand Down Expand Up @@ -201,7 +204,7 @@ export class Waterfall<X, Y> extends Bar<X, number> {
min += startTotal;
max += startTotal;
}
});
}
this._extent = [min, max];
}

Expand Down
34 changes: 22 additions & 12 deletions src/utils/stackingUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import { IAccessor } from "../core/interfaces";
import * as Utils from "./";
import { makeEnum } from "./makeEnum";

const lMemoize = require("lodash.memoize");
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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


export type GenericStackedDatum<D> = {
value: number;
offset: number;
Expand Down Expand Up @@ -75,10 +77,14 @@ export function stack(
datasets.reverse();
}

datasets.forEach((dataset) => {
for (const dataset of datasets) {
const keyToStackedDatum = new Utils.Map<string, StackedDatum>();
dataset.data().forEach((datum, index) => {
const key = normalizeKey(keyAccessor(datum, index, dataset));
const data = dataset.data();
const dataLen = data.length;
for (let index = 0; index < dataLen; index++) {
const datum = data[index];
const accessedKey = keyAccessor(datum, index, dataset);
const key = normalizeKey(accessedKey);
const value = +valueAccessor(datum, index, dataset);
let offset: number;
const offsetMap = (value >= 0) ? positiveOffsets : negativeOffsets;
Expand All @@ -92,14 +98,14 @@ export function stack(
keyToStackedDatum.set(key, {
offset: offset,
value: value,
axisValue: keyAccessor(datum, index, dataset),
axisValue: accessedKey,
originalDatum: datum,
originalDataset: dataset,
originalIndex: index,
});
});
}
datasetToKeyToStackedDatum.set(dataset, keyToStackedDatum);
});
}
return datasetToKeyToStackedDatum;
}

Expand All @@ -120,8 +126,9 @@ export function stackedExtents<D>(stackingResult: GenericStackingResult<D>): {
stackingResult.forEach((stack) => {
stack.forEach((datum: GenericStackedDatum<D>, key) => {
// correctly handle negative bar stacks
const maximalValue = Utils.Math.max([datum.offset + datum.value, datum.offset], datum.offset);
const minimalValue = Utils.Math.min([datum.offset + datum.value, datum.offset], datum.offset);
const offsetValue = datum.offset + datum.value;
const maximalValue = Utils.Math.max([offsetValue, datum.offset], datum.offset);
const minimalValue = Utils.Math.min([offsetValue, datum.offset], datum.offset);

const { axisValue } = datum;

Expand Down Expand Up @@ -153,13 +160,16 @@ export function stackedExtents<D>(stackingResult: GenericStackingResult<D>): {
export function stackedExtent(stackingResult: StackingResult, keyAccessor: IAccessor<any>, filter: IAccessor<boolean>) {
const extents: number[] = [];
stackingResult.forEach((stackedDatumMap: Utils.Map<string, StackedDatum>, dataset: Dataset) => {
dataset.data().forEach((datum, index) => {
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

return -> continue

}
const stackedDatum = stackedDatumMap.get(normalizeKey(keyAccessor(datum, index, dataset)));
extents.push(stackedDatum.value + stackedDatum.offset);
});
}
});
const maxStackExtent = Utils.Math.max(extents, 0);
const minStackExtent = Utils.Math.min(extents, 0);
Expand All @@ -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) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@themadcreator I believe that any key passed in here should be immutable, but given it's typed as any, would want a sanity check?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return String(key);
}
});
4 changes: 4 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3802,6 +3802,10 @@ lodash.keysin@^3.0.0:
lodash.isarguments "^3.0.0"
lodash.isarray "^3.0.0"

lodash.memoize@^4.1.2:
version "4.1.2"
resolved "https://artifactory.palantir.build:443/artifactory/api/npm/all-npm/lodash.memoize/-/lodash.memoize-4.1.2.tgz?dl=https://registry.yarnpkg.com/lodash.memoize/-/lodash.memoize-4.1.2.tgz#bcc6c49a42a2840ed997f323eada5ecd182e0bfe"

lodash.merge@^3.3.2:
version "3.3.2"
resolved "https://registry.yarnpkg.com/lodash.merge/-/lodash.merge-3.3.2.tgz#0d90d93ed637b1878437bb3e21601260d7afe994"
Expand Down