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.

Sign up for GitHub

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

more cache hits for getMaxTickWidth #2687

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

more cache hits for getMaxTickWidth #2687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
69 changes: 40 additions & 29 deletions src/axis.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,40 +309,51 @@ Axis.prototype.textAnchorForY2AxisLabel = function textAnchorForY2AxisLabel() {
var $$ = this.owner;
return this.textAnchorForAxisLabel($$.config.axis_rotated, this.getY2AxisLabelPosition());
};
Axis.prototype.getMaxTickWidth = function getMaxTickWidth(id, withoutRecompute) {
Axis.prototype.getMaxTickWidth = function getMaxTickWidth(id) {
var $$ = this.owner,
maxWidth = 0,
targetsToShow, scale, axis, dummy, svg;
if (withoutRecompute && $$.currentMaxTickWidths[id]) {
return $$.currentMaxTickWidths[id];

const cacheKey = `MaxTickWidth(${id})`;

// return cached value if any
const cachedMaxTickWidth = $$.getFromCache(cacheKey);
if (cachedMaxTickWidth !== undefined) {
return cachedMaxTickWidth;
}
if ($$.svg) {
targetsToShow = $$.filterTargetsToShow($$.data.targets);
if (id === 'y') {
scale = $$.y.copy().domain($$.getYDomain(targetsToShow, 'y'));
axis = this.getYAxis(id, scale, $$.yOrient, $$.yAxisTickValues, false, true, true);
} else if (id === 'y2') {
scale = $$.y2.copy().domain($$.getYDomain(targetsToShow, 'y2'));
axis = this.getYAxis(id, scale, $$.y2Orient, $$.y2AxisTickValues, false, true, true);
} else {
scale = $$.x.copy().domain($$.getXDomain(targetsToShow));
axis = this.getXAxis(scale, $$.xOrient, $$.xAxisTickFormat, $$.xAxisTickValues, false, true, true);
this.updateXAxisTickValues(targetsToShow, axis);
}
dummy = $$.d3.select('body').append('div').classed('c3', true);
svg = dummy.append("svg").style('visibility', 'hidden').style('position', 'fixed').style('top', 0).style('left', 0),
svg.append('g').call(axis).each(function () {
$$.d3.select(this).selectAll('text').each(function () {
var box = this.getBBox();
if (maxWidth < box.width) {
maxWidth = box.width;
}
});
dummy.remove();
});

// bypass if SVG not rendered yet
if (!$$.svg) {
Copy link
Member

Choose a reason for hiding this comment

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

nice early return 👍

return 0;
}

targetsToShow = $$.filterTargetsToShow($$.data.targets);
if (id === 'y') {
scale = $$.y.copy().domain($$.getYDomain(targetsToShow, 'y'));
axis = this.getYAxis(id, scale, $$.yOrient, $$.yAxisTickValues, false, true, true);
} else if (id === 'y2') {
scale = $$.y2.copy().domain($$.getYDomain(targetsToShow, 'y2'));
axis = this.getYAxis(id, scale, $$.y2Orient, $$.y2AxisTickValues, false, true, true);
} else {
scale = $$.x.copy().domain($$.getXDomain(targetsToShow));
axis = this.getXAxis(scale, $$.xOrient, $$.xAxisTickFormat, $$.xAxisTickValues, false, true, true);
this.updateXAxisTickValues(targetsToShow, axis);
}
$$.currentMaxTickWidths[id] = maxWidth <= 0 ? $$.currentMaxTickWidths[id] : maxWidth;
return $$.currentMaxTickWidths[id];
dummy = $$.d3.select('body').append('div').classed('c3', true);
svg = dummy.append("svg").style('visibility', 'hidden').style('position', 'fixed').style('top', 0).style('left', 0),
svg.append('g').call(axis).each(function () {
$$.d3.select(this).selectAll('text').each(function () {
var box = this.getBBox();
if (maxWidth < box.width) {
maxWidth = box.width;
}
});
dummy.remove();
});

$$.addToCache(cacheKey, maxWidth);

return maxWidth;
};

Axis.prototype.updateLabels = function updateLabels(withTransition) {
Expand Down
13 changes: 6 additions & 7 deletions src/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,6 @@ ChartInternal.prototype.initParams = function() {
$$.legendItemWidth = 0;
$$.legendItemHeight = 0;

$$.currentMaxTickWidths = {
x: 0,
y: 0,
y2: 0
};

$$.rotated_padding_left = 30;
$$.rotated_padding_right = config.axis_rotated && !config.axis_x_show ? 0 : 30;
$$.rotated_padding_top = 5;
Expand Down Expand Up @@ -533,6 +527,9 @@ ChartInternal.prototype.redraw = function(options, transitions) {
durationForExit = withTransitionForExit ? duration : 0;
durationForAxis = withTransitionForAxis ? duration : 0;

// reset caches when we are re-drawing the chart
$$.resetCache();
Copy link
Member

@kt3k kt3k Aug 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I guess this resetting of cache might be inconvenient for some future use cases. How about resetting only some groups of caches by the key?

$$.resetCache('MaxTickWidth') // => reset cache which has the key /^\$MaxTickWidth/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I though about this when I tried to use this method for legendItemTextBox too where reseting this cache here would be unnecessary.

I think there will be some kind of tradeoff between ease of use (aka: Forget to reset a cache here & there) & performance.

I think there are 2 families of cached values:

  • The ones that are impacted by an update of data (added, removed targets, etc.)
  • The ones that are impacted by an update of sizing (resize, update of CSS....)


transitions = transitions || $$.axis.generateTransitions(durationForAxis);

// update legend and transform each g
Expand Down Expand Up @@ -762,6 +759,8 @@ ChartInternal.prototype.updateAndRedraw = function(options) {
options.withUpdateOrgXDomain = getOption(options, "withUpdateOrgXDomain", true);
options.withTransitionForExit = false;
options.withTransitionForTransform = getOption(options, "withTransitionForTransform", options.withTransition);
// clear any cache before we update sizes
$$.resetCache();
// MEMO: this needs to be called before updateLegend and it means this ALWAYS needs to be called)
$$.updateSizes();
// MEMO: called in updateLegend in redraw if withLegend
Expand Down Expand Up @@ -1148,7 +1147,7 @@ ChartInternal.prototype.generateWait = function() {
if (!$$.isTabVisible()) {
return;
}

var done = 0;
transitionsToWait.forEach(function(t) {
if (t.empty()) {
Expand Down
14 changes: 7 additions & 7 deletions src/size.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,16 @@ ChartInternal.prototype.getCurrentPaddingBottom = function () {
var config = this.config;
return isValue(config.padding_bottom) ? config.padding_bottom : 0;
};
ChartInternal.prototype.getCurrentPaddingLeft = function (withoutRecompute) {
ChartInternal.prototype.getCurrentPaddingLeft = function () {
var $$ = this, config = $$.config;
if (isValue(config.padding_left)) {
return config.padding_left;
} else if (config.axis_rotated) {
return (!config.axis_x_show || config.axis_x_inner) ? 1 : Math.max(ceil10($$.getAxisWidthByAxisId('x', withoutRecompute)), 40);
return (!config.axis_x_show || config.axis_x_inner) ? 1 : Math.max(ceil10($$.getAxisWidthByAxisId('x')), 40);
} else if (!config.axis_y_show || config.axis_y_inner) { // && !config.axis_rotated
return $$.axis.getYAxisLabelPosition().isOuter ? 30 : 1;
} else {
return ceil10($$.getAxisWidthByAxisId('y', withoutRecompute));
return ceil10($$.getAxisWidthByAxisId('y'));
}
};
ChartInternal.prototype.getCurrentPaddingRight = function () {
Expand Down Expand Up @@ -85,22 +85,22 @@ ChartInternal.prototype.getParentHeight = function () {
};


ChartInternal.prototype.getSvgLeft = function (withoutRecompute) {
ChartInternal.prototype.getSvgLeft = function () {
var $$ = this, config = $$.config,
hasLeftAxisRect = config.axis_rotated || (!config.axis_rotated && !config.axis_y_inner),
leftAxisClass = config.axis_rotated ? CLASS.axisX : CLASS.axisY,
leftAxis = $$.main.select('.' + leftAxisClass).node(),
svgRect = leftAxis && hasLeftAxisRect ? leftAxis.getBoundingClientRect() : {right: 0},
chartRect = $$.selectChart.node().getBoundingClientRect(),
hasArc = $$.hasArcType(),
svgLeft = svgRect.right - chartRect.left - (hasArc ? 0 : $$.getCurrentPaddingLeft(withoutRecompute));
svgLeft = svgRect.right - chartRect.left - (hasArc ? 0 : $$.getCurrentPaddingLeft());
return svgLeft > 0 ? svgLeft : 0;
};


ChartInternal.prototype.getAxisWidthByAxisId = function (id, withoutRecompute) {
ChartInternal.prototype.getAxisWidthByAxisId = function (id) {
var $$ = this, position = $$.axis.getLabelPositionById(id);
return $$.axis.getMaxTickWidth(id, withoutRecompute) + (position.isInner ? 20 : 40);
return $$.axis.getMaxTickWidth(id) + (position.isInner ? 20 : 40);
};
ChartInternal.prototype.getHorizontalAxisHeight = function (axisId) {
var $$ = this, config = $$.config, h = 30;
Expand Down