Skip to content

Commit

Permalink
perf(es/minifier): Do not visit var init multiple times (#9039)
Browse files Browse the repository at this point in the history 
**Description:**

I mistakenly introduced a performance regression with #9032. It makes the minifier visit the initializer of variables multiple times - once while normal visiting and once in `hoist_props_of_vars`. This PR fixes it.
  • Loading branch information
@kdy1
kdy1 committed Jun 12, 2024
1 parent 65bd215 commit 675916c
Show file tree
Hide file tree
Showing 22 changed files with 323 additions and 314 deletions.
34 changes: 30 additions & 4 deletions crates/swc_ecma_minifier/src/compress/optimize/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2955,10 +2955,38 @@ impl VisitMut for Optimizer<'_> {
return false;
}

var.visit_mut_with(self);
true
});

{
// We loop with index to avoid borrow checker issue.
// We use splice so we cannot use for _ in vars
let mut idx = 0;

while idx < vars.len() {
let var = &mut vars[idx];
var.visit_mut_with(self);

// The varaible is dropped.
if var.name.is_invalid() {
vars.remove(idx);
continue;
}

let new = self.hoist_props_of_var(var);

if let Some(new) = new {
let len = new.len();
vars.splice(idx..=idx, new);
idx += len;
} else {
idx += 1;
}
}
}

vars.retain_mut(|var| {
if var.name.is_invalid() {
// It will be inlined.
self.changed = true;
return false;
}
Expand All @@ -2968,8 +2996,6 @@ impl VisitMut for Optimizer<'_> {
true
});

self.hoist_props_of_vars(vars);

let uses_eval = self.data.scopes.get(&self.ctx.scope).unwrap().has_eval_call;

if !uses_eval && !self.ctx.dont_use_prepend_nor_append {
Expand Down
31 changes: 7 additions & 24 deletions crates/swc_ecma_minifier/src/compress/optimize/props.rs
View file
Original file line number Diff line number Diff line change
@@ -1,46 +1,29 @@
use swc_common::{util::take::Take, DUMMY_SP};
use swc_ecma_ast::*;
use swc_ecma_utils::{contains_this_expr, private_ident, prop_name_eq, ExprExt};
use swc_ecma_visit::VisitMutWith;

use super::{unused::PropertyAccessOpts, Optimizer};
use crate::util::deeply_contains_this_expr;

/// Methods related to the option `hoist_props`.
impl Optimizer<'_> {
pub(super) fn hoist_props_of_vars(&mut self, n: &mut Vec<VarDeclarator>) {
pub(super) fn hoist_props_of_var(
&mut self,
n: &mut VarDeclarator,
) -> Option<Vec<VarDeclarator>> {
if !self.options.hoist_props {
log_abort!("hoist_props: option is disabled");
return;
return None;
}
if self.ctx.is_exported {
log_abort!("hoist_props: Exported variable is not hoisted");
return;
return None;
}
if self.ctx.in_top_level() && !self.options.top_level() {
log_abort!("hoist_props: Top-level variable is not hoisted");
return;
}

let mut new = Vec::with_capacity(n.len());
for mut n in n.take() {
if let Some(init) = &mut n.init {
init.visit_mut_with(self);
}

let new_vars = self.hoist_props_of_var(&mut n);

if let Some(new_vars) = new_vars {
new.extend(new_vars);
} else {
new.push(n);
}
return None;
}

*n = new;
}

fn hoist_props_of_var(&mut self, n: &mut VarDeclarator) -> Option<Vec<VarDeclarator>> {
if let Pat::Ident(name) = &mut n.name {
if name.id.span.ctxt == self.marks.top_level_ctxt
&& self.options.top_retain.contains(&name.id.sym)
Expand Down
30 changes: 15 additions & 15 deletions crates/swc_ecma_minifier/tests/benches-full/echarts.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9609,7 +9609,7 @@
], upstreamSignList = [];
assert(resultSourceList && upstreamSignList), this._setLocalSource(resultSourceList, upstreamSignList);
}, SourceManager.prototype._applyTransform = function(upMgrList) {
var source, encodeDefine, sourceList, datasetModel = this._sourceHost, transformOption = datasetModel.get('transform', !0), fromTransformResult = datasetModel.get('fromTransformResult', !0);
var encodeDefine, source, sourceList, datasetModel = this._sourceHost, transformOption = datasetModel.get('transform', !0), fromTransformResult = datasetModel.get('fromTransformResult', !0);
assert(null != fromTransformResult || null != transformOption), null != fromTransformResult && 1 !== upMgrList.length && doThrow('When using `fromTransformResult`, there should be only one upstream dataset');
var upSourceList = [], upstreamSignList = [];
return (each(upMgrList, function(upMgr) {
Expand Down Expand Up @@ -15070,10 +15070,10 @@
return null == precision ? precision = getPrecisionSafe(data.value) || 0 : 'auto' === precision && (precision = this._intervalPrecision), addCommas(round(data.value, precision, !0));
}, IntervalScale.prototype.niceTicks = function(splitNumber, minInterval, maxInterval) {
splitNumber = splitNumber || 5;
var splitNumber1, result, span, interval, precision, niceTickExtent, extent = this._extent, span1 = extent[1] - extent[0];
var splitNumber1, result, span, interval, precision, extent = this._extent, span1 = extent[1] - extent[0];
if (isFinite(span1)) {
span1 < 0 && (span1 = -span1, extent.reverse());
var result1 = (splitNumber1 = splitNumber, result = {}, span = extent[1] - extent[0], interval = result.interval = nice(span / splitNumber1, !0), null != minInterval && interval < minInterval && (interval = result.interval = minInterval), null != maxInterval && interval > maxInterval && (interval = result.interval = maxInterval), precision = result.intervalPrecision = getPrecisionSafe(interval) + 2, isFinite((niceTickExtent = result.niceTickExtent = [
var niceTickExtent, result1 = (splitNumber1 = splitNumber, result = {}, span = extent[1] - extent[0], interval = result.interval = nice(span / splitNumber1, !0), null != minInterval && interval < minInterval && (interval = result.interval = minInterval), null != maxInterval && interval > maxInterval && (interval = result.interval = maxInterval), precision = result.intervalPrecision = getPrecisionSafe(interval) + 2, isFinite((niceTickExtent = result.niceTickExtent = [
round(Math.ceil(extent[0] / interval) * interval, precision),
round(Math.floor(extent[1] / interval) * interval, precision)
])[0]) || (niceTickExtent[0] = extent[0]), isFinite(niceTickExtent[1]) || (niceTickExtent[1] = extent[1]), clamp(niceTickExtent, 0, extent), clamp(niceTickExtent, 1, extent), niceTickExtent[0] > niceTickExtent[1] && (niceTickExtent[0] = niceTickExtent[1]), result);
Expand Down Expand Up @@ -16083,7 +16083,7 @@
return ('category' === this.type ? (result = makeCategoryLabelsActually(this, labelModel = this.getLabelModel()), !labelModel.get('show') || this.scale.isBlank() ? {
labels: [],
labelCategoryInterval: result.labelCategoryInterval
} : result) : (ticks = (axis = this).scale.getTicks(), labelFormatter = makeLabelFormatter(axis), {
} : result) : (axis = this, ticks = axis.scale.getTicks(), labelFormatter = makeLabelFormatter(axis), {
labels: map(ticks, function(tick, idx) {
return {
formattedLabel: labelFormatter(tick, idx),
Expand Down Expand Up @@ -19281,13 +19281,13 @@
this._ctx = null;
for(var i = 0; i < points.length;){
var x = points[i++], y = points[i++];
isNaN(x) || isNaN(y) || this.softClipShape && !this.softClipShape.contain(x, y) || (symbolProxyShape.x = x - size[0] / 2, symbolProxyShape.y = y - size[1] / 2, symbolProxyShape.width = size[0], symbolProxyShape.height = size[1], symbolProxy.buildPath(path, symbolProxyShape, !0));
!(isNaN(x) || isNaN(y)) && (!this.softClipShape || this.softClipShape.contain(x, y)) && (symbolProxyShape.x = x - size[0] / 2, symbolProxyShape.y = y - size[1] / 2, symbolProxyShape.width = size[0], symbolProxyShape.height = size[1], symbolProxy.buildPath(path, symbolProxyShape, !0));
}
}, LargeSymbolPath.prototype.afterBrush = function() {
var shape = this.shape, points = shape.points, size = shape.size, ctx = this._ctx;
if (ctx) for(var i = 0; i < points.length;){
var x = points[i++], y = points[i++];
isNaN(x) || isNaN(y) || this.softClipShape && !this.softClipShape.contain(x, y) || ctx.fillRect(x - size[0] / 2, y - size[1] / 2, size[0], size[1]);
!(isNaN(x) || isNaN(y)) && (!this.softClipShape || this.softClipShape.contain(x, y)) && ctx.fillRect(x - size[0] / 2, y - size[1] / 2, size[0], size[1]);
}
}, LargeSymbolPath.prototype.findDataIndex = function(x, y) {
for(var shape = this.shape, points = shape.points, size = shape.size, w = Math.max(size[0], 4), h = Math.max(size[1], 4), idx = points.length / 2 - 1; idx >= 0; idx--){
Expand Down Expand Up @@ -20188,11 +20188,11 @@
axisName: function(opt, axisModel, group, transformGroup) {
var labelLayout, axisNameAvailableWidth, name = retrieve(opt.axisName, axisModel.get('name'));
if (name) {
var textAlign, textVerticalAlign, rotationDiff, inverse, onLeft, nameLocation = axisModel.get('nameLocation'), nameDirection = opt.nameDirection, textStyleModel = axisModel.getModel('nameTextStyle'), gap = axisModel.get('nameGap') || 0, extent = axisModel.axis.getExtent(), gapSignal = extent[0] > extent[1] ? -1 : 1, pos = [
var rotation, textAlign, textVerticalAlign, rotationDiff, inverse, onLeft, nameLocation = axisModel.get('nameLocation'), nameDirection = opt.nameDirection, textStyleModel = axisModel.getModel('nameTextStyle'), gap = axisModel.get('nameGap') || 0, extent = axisModel.axis.getExtent(), gapSignal = extent[0] > extent[1] ? -1 : 1, pos = [
'start' === nameLocation ? extent[0] - gapSignal * gap : 'end' === nameLocation ? extent[1] + gapSignal * gap : (extent[0] + extent[1]) / 2,
isNameLocationCenter(nameLocation) ? opt.labelOffset + nameDirection * gap : 0
], nameRotation = axisModel.get('nameRotate');
null != nameRotation && (nameRotation = nameRotation * PI$5 / 180), isNameLocationCenter(nameLocation) ? labelLayout = AxisBuilder.innerTextLayout(opt.rotation, null != nameRotation ? nameRotation : opt.rotation, nameDirection) : (rotationDiff = remRadian((nameRotation || 0) - opt.rotation), inverse = extent[0] > extent[1], onLeft = 'start' === nameLocation && !inverse || 'start' !== nameLocation && inverse, isRadianAroundZero(rotationDiff - PI$5 / 2) ? (textVerticalAlign = onLeft ? 'bottom' : 'top', textAlign = 'center') : isRadianAroundZero(rotationDiff - 1.5 * PI$5) ? (textVerticalAlign = onLeft ? 'top' : 'bottom', textAlign = 'center') : (textVerticalAlign = 'middle', textAlign = rotationDiff < 1.5 * PI$5 && rotationDiff > PI$5 / 2 ? onLeft ? 'left' : 'right' : onLeft ? 'right' : 'left'), labelLayout = {
null != nameRotation && (nameRotation = nameRotation * PI$5 / 180), isNameLocationCenter(nameLocation) ? labelLayout = AxisBuilder.innerTextLayout(opt.rotation, null != nameRotation ? nameRotation : opt.rotation, nameDirection) : (rotation = opt.rotation, rotationDiff = remRadian((nameRotation || 0) - rotation), inverse = extent[0] > extent[1], onLeft = 'start' === nameLocation && !inverse || 'start' !== nameLocation && inverse, isRadianAroundZero(rotationDiff - PI$5 / 2) ? (textVerticalAlign = onLeft ? 'bottom' : 'top', textAlign = 'center') : isRadianAroundZero(rotationDiff - 1.5 * PI$5) ? (textVerticalAlign = onLeft ? 'top' : 'bottom', textAlign = 'center') : (textVerticalAlign = 'middle', textAlign = rotationDiff < 1.5 * PI$5 && rotationDiff > PI$5 / 2 ? onLeft ? 'left' : 'right' : onLeft ? 'right' : 'left'), labelLayout = {
rotation: rotationDiff,
textAlign: textAlign,
textVerticalAlign: textVerticalAlign
Expand Down Expand Up @@ -22924,19 +22924,19 @@
],
[
x + itemWidth,
0 + itemHeight
y + itemHeight
],
[
head ? x : x - 5,
0 + itemHeight
y + itemHeight
]
];
return tail || points.splice(2, 0, [
x + itemWidth + 5,
0 + itemHeight / 2
y + itemHeight / 2
]), head || points.push([
x,
0 + itemHeight / 2
y + itemHeight / 2
]), points;
}(lastX, 0, itemWidth, height, i === renderList.length - 1, 0 === i)
},
Expand Down Expand Up @@ -28681,7 +28681,7 @@
]);
}, LinesView.prototype._clearLayer = function(api) {
var zr = api.getZr();
'svg' === zr.painter.getType() || null == this._lastZlevel || zr.painter.getLayer(this._lastZlevel).clear(!0);
'svg' !== zr.painter.getType() && null != this._lastZlevel && zr.painter.getLayer(this._lastZlevel).clear(!0);
}, LinesView.prototype.remove = function(ecModel, api) {
this._lineDraw && this._lineDraw.remove(), this._lineDraw = null, this._clearLayer(api);
}, LinesView.type = 'lines', LinesView;
Expand Down Expand Up @@ -34689,7 +34689,7 @@
return null !== _super && _super.apply(this, arguments) || this;
}
return __extends(DataView, _super), DataView.prototype.onclick = function(ecModel, api) {
var seriesGroupByCategoryAxis, otherSeries, meta, result, groups, tables, container = api.getDom(), model = this.model;
var seriesGroupByCategoryAxis, otherSeries, meta, groups, tables, result, container = api.getDom(), model = this.model;
this._dom && container.removeChild(this._dom);
var root = document.createElement('div');
root.style.cssText = 'position:absolute;left:5px;top:5px;bottom:5px;right:5px;', root.style.backgroundColor = model.get('backgroundColor') || '#fff';
Expand Down Expand Up @@ -35481,7 +35481,7 @@
}, TooltipHTMLContent.prototype.show = function(tooltipModel, nearPointColor) {
clearTimeout(this._hideTimeout), clearTimeout(this._longHideTimeout);
var enableTransition, onlyFade, cssText, transitionDuration, backgroundColor, shadowBlur, shadowColor, shadowOffsetX, shadowOffsetY, textStyleModel, padding, transitionCurve, transitionOption, transitionText, cssText1, fontSize, color, shadowColor1, shadowBlur1, shadowOffsetX1, shadowOffsetY1, el = this.el, style = el.style, styleCoord = this._styleCoord;
el.innerHTML ? style.cssText = gCssText + (enableTransition = !this._firstShow, onlyFade = this._longHide, cssText = [], transitionDuration = tooltipModel.get('transitionDuration'), backgroundColor = tooltipModel.get('backgroundColor'), shadowBlur = tooltipModel.get('shadowBlur'), shadowColor = tooltipModel.get('shadowColor'), shadowOffsetX = tooltipModel.get('shadowOffsetX'), shadowOffsetY = tooltipModel.get('shadowOffsetY'), textStyleModel = tooltipModel.getModel('textStyle'), padding = getPaddingFromTooltipModel(tooltipModel, 'html'), cssText.push('box-shadow:' + shadowOffsetX + "px " + shadowOffsetY + "px " + shadowBlur + "px " + shadowColor), enableTransition && transitionDuration && cssText.push((transitionText = "opacity" + (transitionOption = " " + transitionDuration / 2 + "s " + (transitionCurve = 'cubic-bezier(0.23,1,0.32,1)')) + ",visibility" + transitionOption, onlyFade || (transitionOption = " " + transitionDuration + "s " + transitionCurve, transitionText += env.transformSupported ? "," + TRANSFORM_VENDOR + transitionOption : ",left" + transitionOption + ",top" + transitionOption), CSS_TRANSITION_VENDOR + ':' + transitionText)), backgroundColor && (env.canvasSupported ? cssText.push('background-color:' + backgroundColor) : (cssText.push('background-color:#' + toHex(backgroundColor)), cssText.push('filter:alpha(opacity=70)'))), each([
el.innerHTML ? style.cssText = gCssText + (enableTransition = !this._firstShow, onlyFade = this._longHide, cssText = [], transitionDuration = tooltipModel.get('transitionDuration'), backgroundColor = tooltipModel.get('backgroundColor'), shadowBlur = tooltipModel.get('shadowBlur'), shadowColor = tooltipModel.get('shadowColor'), shadowOffsetX = tooltipModel.get('shadowOffsetX'), shadowOffsetY = tooltipModel.get('shadowOffsetY'), textStyleModel = tooltipModel.getModel('textStyle'), padding = getPaddingFromTooltipModel(tooltipModel, 'html'), cssText.push('box-shadow:' + (shadowOffsetX + "px " + shadowOffsetY + "px ") + shadowBlur + "px " + shadowColor), enableTransition && transitionDuration && cssText.push((transitionText = "opacity" + (transitionOption = " " + transitionDuration / 2 + "s " + (transitionCurve = 'cubic-bezier(0.23,1,0.32,1)')) + ",visibility" + transitionOption, onlyFade || (transitionOption = " " + transitionDuration + "s " + transitionCurve, transitionText += env.transformSupported ? "," + TRANSFORM_VENDOR + transitionOption : ",left" + transitionOption + ",top" + transitionOption), CSS_TRANSITION_VENDOR + ':' + transitionText)), backgroundColor && (env.canvasSupported ? cssText.push('background-color:' + backgroundColor) : (cssText.push('background-color:#' + toHex(backgroundColor)), cssText.push('filter:alpha(opacity=70)'))), each([
'width',
'color',
'radius'
Expand Down
3 changes: 2 additions & 1 deletion crates/swc_ecma_minifier/tests/benches-full/jquery.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,8 @@
}
function createDisabledPseudo(disabled) {
return function(elem) {
return "form" in elem ? elem.parentNode && !1 === elem.disabled ? "label" in elem ? "label" in elem.parentNode ? elem.parentNode.disabled === disabled : elem.disabled === disabled : elem.isDisabled === disabled || !disabled !== elem.isDisabled && inDisabledFieldset(elem) === disabled : elem.disabled === disabled : "label" in elem && elem.disabled === disabled;
if ("form" in elem) return elem.parentNode && !1 === elem.disabled ? "label" in elem ? "label" in elem.parentNode ? elem.parentNode.disabled === disabled : elem.disabled === disabled : elem.isDisabled === disabled || !disabled !== elem.isDisabled && inDisabledFieldset(elem) === disabled : elem.disabled === disabled;
return "label" in elem && elem.disabled === disabled;
};
}
function createPositionalPseudo(fn) {
Expand Down
Loading

0 comments on commit 675916c

Please sign in to comment.