Skip to content
This repository has been archived by the owner on Feb 19, 2022. It is now read-only.

Perf optimizations #27

Closed
wants to merge 3 commits into from
Closed

Perf optimizations #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4009364
Add multi-argument memoization function.
divmain Feb 18, 2016
648877a
Simplify style merging for cases where non-default style in unspecified.
divmain Feb 18, 2016
6ba3388
Lint and tests.
divmain Feb 18, 2016
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
9 changes: 6 additions & 3 deletions src/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,14 @@ module.exports = {
};
},

getStyles(props, defaultStyles) {
const style = props.style || defaultStyles;
getStyles(style, defaultStyles, height, width) { // eslint-disable-line max-params
if (!style) {
return merge({}, defaultStyles, { parent: { height, width } });
}

const {data, labels, parent} = style;
return {
parent: merge({}, defaultStyles.parent, parent, {height: props.height, width: props.width}),
parent: merge({}, defaultStyles.parent, parent, { height, width }),
labels: merge({}, defaultStyles.labels, labels),
data: merge({}, defaultStyles.data, data)
};
Expand Down
4 changes: 3 additions & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ import * as Log from "./log";
import * as Style from "./style";
import * as Type from "./type";
import * as PropTypes from "./prop-types";
import * as Perf from "./perf";

export default {
Collection,
Helpers,
Log,
Style,
Type,
PropTypes
PropTypes,
Perf
};
12 changes: 12 additions & 0 deletions src/perf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
export const memoize = function (fn) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you paste a code comment with a reference / links to where we're getting our memoize function from? There are a ton of battle-tested examples out there, and hopefully we can leverage a good, well-trodden path here.

Copy link
Author

Choose a reason for hiding this comment

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

Happy to make a change here if you think it is necessary, but let me explain my thinking. If you look at the memoize function that is included with Lodash, there isn't much to it. If a resolver is included, all arguments will be passed (this is not the case with Underscore's memoize), which makes using Lodash#memoize a possibility.

However, the resolver key-generation will consist of basically all of the code in this PR's memoize, plus overhead of an additional fn.apply. Since the motivation for this PR is to improve performance, and because this memoize is in the hot code path, I wanted to reduce any unnecessary overhead.

In other words, I'm fine changing this to _.memoize(fn, /* all the code in perf.js */), but it doesn't seem to buy us anything and it will mean a slight perf hit.

const cache = {};
return function () {
const args = Array.prototype.slice.call(arguments);
const hash = args.map((arg) => {
return (typeof arg === "string" || typeof arg === "number") ? arg : JSON.stringify(arg);
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work for function args; it might be worth noting in a comment.

Copy link
Author

Choose a reason for hiding this comment

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

Good thought.

}).join("~");
return hash in cache ?
cache[hash] :
cache[hash] = fn.apply(this, args); // eslint-disable-line no-invalid-this
};
};
5 changes: 3 additions & 2 deletions test/client/spec/helpers.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,9 @@ describe("helpers", () => {
};
it("merges styles", () => {
const style = {data: {fill: "red"}, labels: {fontSize: 12}};
const props = {style, width: 500, height: 500};
const styles = Helpers.getStyles(props, defaultStyles);
const width = 500;
const height = 500;
const styles = Helpers.getStyles(style, defaultStyles, height, width);
expect(styles.parent).to.deep.equal({border: "black", width: 500, height: 500});
expect(styles.data).to.deep.equal({fill: "red", stroke: "black"});
expect(styles.labels).to.deep.equal({fontSize: 12, fontFamily: "Helvetica"});
Expand Down