Skip to content

Commit 50c326b

Browse files
LorfdailLorfdail
authored
perf: optimiztions to reduce regression test runtime (#2135)
Small group of changes that at reduce the regression test runtime by about 14%. Motivation for this change in particular were the large PNGs usually allocated for even the smallest SVGs in the test data which both increases the time spent allocating as well as the time pixelmatch would take for the comparison. The other changes improve not only the test runtime but also the normal SVG processing given the input data has cases caught by removeHiddenElems which also has the by far most significant of the 3 changes. All the changes done for baseline performance optimization are very unlikely to cause any harm and would need specific sets of input SVGs mostly consisting of visibility checks regarding the element style to impact performance in a negative way. In my local tests these changes gave an additional ~7-8% time reduction with the svgo data set.
1 parent 1f33cbe commit 50c326b

File tree

6 files changed

+100
-101
lines changed

6 files changed

+100
-101
lines changed

‎.github/workflows/regression.yml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,14 @@ jobs:
3131
# We use upload/artifacts instead of outputs because our regression test
3232
# report can exceed 1 MB which is a limit GitHub imposes.
3333
- uses: actions/upload-artifact@v4
34+
if: success() || failure()
3435
with:
3536
name: svgo-test-report-${{ github.sha }}
3637
path: /tmp/svgo.${{ github.sha }}/svgo-test-report.json
3738
if-no-files-found: error
3839
retention-days: 1
3940
delta:
41+
if: success() || failure()
4042
runs-on: ubuntu-latest
4143
needs:
4244
- regression

‎lib/svgo/tools.js‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,14 @@ export const removeLeadingZero = (value) => {
145145
return strValue;
146146
};
147147

148+
const hasScriptsEventAttrs = [
149+
...attrsGroups.animationEvent,
150+
...attrsGroups.documentEvent,
151+
...attrsGroups.documentElementEvent,
152+
...attrsGroups.globalEvent,
153+
...attrsGroups.graphicalEvent,
154+
];
155+
148156
/**
149157
* If the current node contains any scripts. This does not check parents or
150158
* children of the node, only the properties and attributes of the node itself.
@@ -170,15 +178,7 @@ export const hasScripts = (node) => {
170178
}
171179
}
172180

173-
const eventAttrs = [
174-
...attrsGroups.animationEvent,
175-
...attrsGroups.documentEvent,
176-
...attrsGroups.documentElementEvent,
177-
...attrsGroups.globalEvent,
178-
...attrsGroups.graphicalEvent,
179-
];
180-
181-
return eventAttrs.some((attr) => node.attributes[attr] != null);
181+
return hasScriptsEventAttrs.some((attr) => node.attributes[attr] != null);
182182
};
183183

184184
/**

‎plugins/convertPathData.js‎

Lines changed: 18 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -230,8 +230,7 @@ const convertToRelative = (pathData) => {
230230
cursor[1] += args[1];
231231
start[0] = cursor[0];
232232
start[1] = cursor[1];
233-
}
234-
if (command === 'M') {
233+
} else if (command === 'M') {
235234
// M → m
236235
// skip first moveto
237236
if (i !== 0) {
@@ -247,11 +246,10 @@ const convertToRelative = (pathData) => {
247246
}
248247

249248
// lineto (x y)
250-
if (command === 'l') {
249+
else if (command === 'l') {
251250
cursor[0] += args[0];
252251
cursor[1] += args[1];
253-
}
254-
if (command === 'L') {
252+
} else if (command === 'L') {
255253
// L → l
256254
command = 'l';
257255
args[0] -= cursor[0];
@@ -261,33 +259,30 @@ const convertToRelative = (pathData) => {
261259
}
262260

263261
// horizontal lineto (x)
264-
if (command === 'h') {
262+
else if (command === 'h') {
265263
cursor[0] += args[0];
266-
}
267-
if (command === 'H') {
264+
} else if (command === 'H') {
268265
// H → h
269266
command = 'h';
270267
args[0] -= cursor[0];
271268
cursor[0] += args[0];
272269
}
273270

274271
// vertical lineto (y)
275-
if (command === 'v') {
272+
else if (command === 'v') {
276273
cursor[1] += args[0];
277-
}
278-
if (command === 'V') {
274+
} else if (command === 'V') {
279275
// V → v
280276
command = 'v';
281277
args[0] -= cursor[1];
282278
cursor[1] += args[0];
283279
}
284280

285281
// curveto (x1 y1 x2 y2 x y)
286-
if (command === 'c') {
282+
else if (command === 'c') {
287283
cursor[0] += args[4];
288284
cursor[1] += args[5];
289-
}
290-
if (command === 'C') {
285+
} else if (command === 'C') {
291286
// C → c
292287
command = 'c';
293288
args[0] -= cursor[0];
@@ -301,11 +296,10 @@ const convertToRelative = (pathData) => {
301296
}
302297

303298
// smooth curveto (x2 y2 x y)
304-
if (command === 's') {
299+
else if (command === 's') {
305300
cursor[0] += args[2];
306301
cursor[1] += args[3];
307-
}
308-
if (command === 'S') {
302+
} else if (command === 'S') {
309303
// S → s
310304
command = 's';
311305
args[0] -= cursor[0];
@@ -317,11 +311,10 @@ const convertToRelative = (pathData) => {
317311
}
318312

319313
// quadratic Bézier curveto (x1 y1 x y)
320-
if (command === 'q') {
314+
else if (command === 'q') {
321315
cursor[0] += args[2];
322316
cursor[1] += args[3];
323-
}
324-
if (command === 'Q') {
317+
} else if (command === 'Q') {
325318
// Q → q
326319
command = 'q';
327320
args[0] -= cursor[0];
@@ -333,11 +326,10 @@ const convertToRelative = (pathData) => {
333326
}
334327

335328
// smooth quadratic Bézier curveto (x y)
336-
if (command === 't') {
329+
else if (command === 't') {
337330
cursor[0] += args[0];
338331
cursor[1] += args[1];
339-
}
340-
if (command === 'T') {
332+
} else if (command === 'T') {
341333
// T → t
342334
command = 't';
343335
args[0] -= cursor[0];
@@ -347,11 +339,10 @@ const convertToRelative = (pathData) => {
347339
}
348340

349341
// elliptical arc (rx ry x-axis-rotation large-arc-flag sweep-flag x y)
350-
if (command === 'a') {
342+
else if (command === 'a') {
351343
cursor[0] += args[5];
352344
cursor[1] += args[6];
353-
}
354-
if (command === 'A') {
345+
} else if (command === 'A') {
355346
// A → a
356347
command = 'a';
357348
args[5] -= cursor[0];
@@ -361,7 +352,7 @@ const convertToRelative = (pathData) => {
361352
}
362353

363354
// closepath
364-
if (command === 'Z' || command === 'z') {
355+
else if (command === 'Z' || command === 'z') {
365356
// reset cursor
366357
cursor[0] = start[0];
367358
cursor[1] = start[1];

‎plugins/removeHiddenElems.js‎

Lines changed: 58 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -190,38 +190,6 @@ export const fn = (root, params) => {
190190
}
191191
}
192192

193-
// Removes hidden elements
194-
// https://www.w3schools.com/cssref/pr_class_visibility.asp
195-
const computedStyle = computeStyle(stylesheet, node);
196-
if (
197-
isHidden &&
198-
computedStyle.visibility &&
199-
computedStyle.visibility.type === 'static' &&
200-
computedStyle.visibility.value === 'hidden' &&
201-
// keep if any descendant enables visibility
202-
querySelector(node, '[visibility=visible]') == null
203-
) {
204-
removeElement(node, parentNode);
205-
return;
206-
}
207-
208-
// display="none"
209-
//
210-
// https://www.w3.org/TR/SVG11/painting.html#DisplayProperty
211-
// "A value of display: none indicates that the given element
212-
// and its children shall not be rendered directly"
213-
if (
214-
displayNone &&
215-
computedStyle.display &&
216-
computedStyle.display.type === 'static' &&
217-
computedStyle.display.value === 'none' &&
218-
// markers with display: none still rendered
219-
node.name !== 'marker'
220-
) {
221-
removeElement(node, parentNode);
222-
return;
223-
}
224-
225193
// Circles with zero radius
226194
//
227195
// https://www.w3.org/TR/SVG11/shapes.html#CircleElementRAttribute
@@ -363,32 +331,6 @@ export const fn = (root, params) => {
363331
return;
364332
}
365333

366-
// Path with empty data
367-
//
368-
// https://www.w3.org/TR/SVG11/paths.html#DAttribute
369-
//
370-
// <path d=""/>
371-
if (pathEmptyD && node.name === 'path') {
372-
if (node.attributes.d == null) {
373-
removeElement(node, parentNode);
374-
return;
375-
}
376-
const pathData = parsePathData(node.attributes.d);
377-
if (pathData.length === 0) {
378-
removeElement(node, parentNode);
379-
return;
380-
}
381-
// keep single point paths for markers
382-
if (
383-
pathData.length === 1 &&
384-
computedStyle['marker-start'] == null &&
385-
computedStyle['marker-end'] == null
386-
) {
387-
removeElement(node, parentNode);
388-
return;
389-
}
390-
}
391-
392334
// Polyline with empty points
393335
//
394336
// https://www.w3.org/TR/SVG11/shapes.html#PolylineElementPointsAttribute
@@ -417,6 +359,64 @@ export const fn = (root, params) => {
417359
return;
418360
}
419361

362+
// Removes hidden elements
363+
// https://www.w3schools.com/cssref/pr_class_visibility.asp
364+
const computedStyle = computeStyle(stylesheet, node);
365+
if (
366+
isHidden &&
367+
computedStyle.visibility &&
368+
computedStyle.visibility.type === 'static' &&
369+
computedStyle.visibility.value === 'hidden' &&
370+
// keep if any descendant enables visibility
371+
querySelector(node, '[visibility=visible]') == null
372+
) {
373+
removeElement(node, parentNode);
374+
return;
375+
}
376+
377+
// display="none"
378+
//
379+
// https://www.w3.org/TR/SVG11/painting.html#DisplayProperty
380+
// "A value of display: none indicates that the given element
381+
// and its children shall not be rendered directly"
382+
if (
383+
displayNone &&
384+
computedStyle.display &&
385+
computedStyle.display.type === 'static' &&
386+
computedStyle.display.value === 'none' &&
387+
// markers with display: none still rendered
388+
node.name !== 'marker'
389+
) {
390+
removeElement(node, parentNode);
391+
return;
392+
}
393+
394+
// Path with empty data
395+
//
396+
// https://www.w3.org/TR/SVG11/paths.html#DAttribute
397+
//
398+
// <path d=""/>
399+
if (pathEmptyD && node.name === 'path') {
400+
if (node.attributes.d == null) {
401+
removeElement(node, parentNode);
402+
return;
403+
}
404+
const pathData = parsePathData(node.attributes.d);
405+
if (pathData.length === 0) {
406+
removeElement(node, parentNode);
407+
return;
408+
}
409+
// keep single point paths for markers
410+
if (
411+
pathData.length === 1 &&
412+
computedStyle['marker-start'] == null &&
413+
computedStyle['marker-end'] == null
414+
) {
415+
removeElement(node, parentNode);
416+
return;
417+
}
418+
}
419+
420420
for (const [name, value] of Object.entries(node.attributes)) {
421421
const ids = findReferences(name, value);
422422

‎test/regression/compare.js‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ const HEIGHT = 720;
2222
/** @type {import('playwright').PageScreenshotOptions} */
2323
const screenshotOptions = {
2424
omitBackground: true,
25-
clip: { x: 0, y: 0, width: WIDTH, height: HEIGHT },
2625
animations: 'disabled',
2726
};
2827

@@ -63,21 +62,26 @@ const runTests = async (list) => {
6362
*/
6463
const processFile = async (page, name) => {
6564
await page.goto(`file://${path.join(REGRESSION_FIXTURES_PATH, name)}`);
66-
const originalBuffer = await page.screenshot(screenshotOptions);
65+
let element = await page.waitForSelector('svg');
66+
const originalBuffer = await element.screenshot(screenshotOptions);
6767

6868
await page.goto(`file://${path.join(REGRESSION_OPTIMIZED_PATH, name)}`);
69-
const optimizedBufferPromise = page.screenshot(screenshotOptions);
69+
element = await page.waitForSelector('svg');
70+
const optimizedBufferPromise = element.screenshot(screenshotOptions);
7071

71-
const writeDiffs = process.env.NO_DIFF == null;
72-
const diff = writeDiffs ? new PNG({ width: WIDTH, height: HEIGHT }) : null;
7372
const originalPng = PNG.sync.read(originalBuffer);
7473
const optimizedPng = PNG.sync.read(await optimizedBufferPromise);
74+
const writeDiffs = process.env.NO_DIFF == null;
75+
const diff = writeDiffs
76+
? new PNG({ width: originalPng.width, height: originalPng.height })
77+
: null;
78+
7579
const matched = pixelmatch(
7680
originalPng.data,
7781
optimizedPng.data,
7882
diff?.data,
79-
WIDTH,
80-
HEIGHT,
83+
originalPng.width,
84+
originalPng.height,
8185
);
8286

8387
// ignore small aliasing issues

‎test/regression/lists/expect-mismatch.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,5 +158,7 @@ W3C_SVG_11_TestSuite/svg/filters-displace-01-f.svg
158158
W3C_SVG_11_TestSuite/svg/fonts-kern-01-t.svg
159159
W3C_SVG_11_TestSuite/svg/interact-pevents-01-b.svg
160160
W3C_SVG_11_TestSuite/svg/struct-cond-03-t.svg
161+
wikimedia-commons/Aegean_sea_Anatolia_and_Armenian_highlands_regions_large_topographic_basemap.svg
161162
wikimedia-commons/Italy_-_Regions_and_provinces.svg
162163
wikimedia-commons/Saariston_Rengastie_route_labels.svg
164+
wikimedia-commons/Spain_languages-de.svg

0 commit comments

Comments
 (0)