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

perf(): rm runtime RegExp usages #9802

Merged
merged 5 commits into from
Jun 3, 2024
Merged

perf(): rm runtime RegExp usages #9802

merged 5 commits into from
Jun 3, 2024

Conversation

ShaMan123
Copy link
Contributor

Description

During a perf profiling I noticed that regexp creation was showing up due to use of parsePath so I looked into the code found deoptimized code and deoptimized use of regexp.
Therefore, I moved all regexp creation from runtime to const values and simplified a bit of parsePath

In Action

Copy link

codesandbox bot commented Apr 17, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link
Contributor

github-actions bot commented Apr 17, 2024

Build Stats

file / KB (diff) bundled minified
fabric 919.551 (+0.104) 306.414 (+0.020)

@@ -1,3 +1,6 @@
/**
* Do not use in runtime
*/
Copy link
Member

Choose a reason for hiding this comment

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

This function is already used at runtime during svg parsing.
Which is the concept you wanted to add here? to do not add it in loops?

@asturur
Copy link
Member

asturur commented Apr 17, 2024

I think this change is fine, we moved out regexp from always-allocated to runtime-allocated by choice, we should remind it when we roll it back, if that hurts performance is fine.

I need to understand what you mean when you say deoptmized code and use of regexp, deoptimization is usually when the v8 needs to rollback to sort of code parsing rather than compiled, is that what you mean?

Please attach before/after flamegraph for reference

new RegExp(`${filteredGroups.join(' ?')} ?$`),
''
);
match = match.slice(value ? value.length : 0);
Copy link
Member

Choose a reason for hiding this comment

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

the changes to the path parsing code please either add a very detailed explanation or let's not do them.

if (!paramArr) {
break;
}
// ignore undefined match groups
const filteredGroups = paramArr.filter((g) => g);
const filteredGroups = paramArr.filter(Boolean);
Copy link
Member

@asturur asturur Apr 17, 2024

Choose a reason for hiding this comment

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

This equal to call Boolean as Boolean(g, index, paramArr) this is usually suggested to avoid, i would avoid

Copy link
Contributor

@jiayihu jiayihu May 4, 2024

Choose a reason for hiding this comment

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

I'd avoid filter(Boolean) also because from TS 5.5 it will be able to automatically type narrow .filter(g => !!g) to defined values, but .filter(Boolean) won't work

microsoft/TypeScript#50387 (comment)

@asturur asturur marked this pull request as draft April 25, 2024 14:45
Copy link
Contributor

github-actions bot commented Jun 3, 2024

Coverage after merging perf/regex-const into master will be

84.33%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
index.node.ts14.29%100%0%25%23, 26, 29, 41, 44, 47
src
   ClassRegistry.ts90.32%61.54%100%97.73%31, 52
   Collection.ts78.47%42.62%87.10%85.82%130, 138, 153, 155–157, 159, 169–170, 181, 197, 215, 217, 228, 243, 254, 265, 270, 279, 281, 286–287, 302, 304, 309–310, 329, 333–334, 338–344, 346–348, 350
   CommonMethods.ts91.43%71.43%100%96%50, 52
   Intersection.ts85.25%48.91%100%97.30%184–188, 190, 228, 237, 239, 289, 297, 297
   Observable.ts79.89%54.55%93.75%87.10%136, 145, 148, 160, 162, 167, 68–70, 72, 76, 80, 84–85, 87–91
   Point.ts90.27%61.22%100%93.60%104, 117, 148, 157, 179, 197, 206, 216, 225, 236–239, 259, 285, 297, 317, 328, 341, 349, 359, 95
   Shadow.ts89.39%78.26%100%90.19%145, 148, 150–155, 164, 201, 204, 211, 228–235, 239–240
   cache.ts84.88%45.45%100%90.14%57, 59, 71–72, 74–77
   config.ts87.73%55%66.67%94.03%132, 134–137, 139, 142–143, 147, 152
   constants.ts100%100%100%100%
src/LayoutManager
   ActiveSelectionLayoutManager.ts93.33%76.92%85.71%100%
   LayoutManager.ts90.54%65.06%76.92%99.29%269, 333, 344
   constants.ts100%100%100%100%
   index.ts48.57%37.50%80%66.67%1, 1, 1–2, 2, 2–3, 3, 3–4, 4, 4–5, 5, 5, 5–6
   types.ts100%100%100%100%
src/LayoutManager/LayoutStrategies
   ClipPathLayout.ts73.08%50%100%78.95%39, 41–44, 46–48, 57–58, 66–69
   FitContentLayout.ts100%100%100%100%
   FixedLayout.ts85.71%20%100%100%23, 23
   LayoutStrategy.ts87.60%55.56%100%96.55%46, 54, 72, 72, 74
   utils.ts72.58%50%100%78.72%29–32, 34–35, 40–44
src/Pattern
   Pattern.ts70.18%90.91%80%65.95%105–107, 114, 118–119, 119–122, 130–138, 140–141, 143, 153–164, 174, 176–181, 183–188, 190–199, 204–205, 207–209, 211, 33, 37
src/brushes
   BaseBrush.ts89.33%91.67%100%88.55%110, 115, 124–125, 130, 135, 143, 146, 155–160, 99
   CircleBrush.ts52.10%12.50%12.50%58.25%100–108, 108–118, 122, 130–139, 55, 67, 69, 76, 76, 78–79, 79, 83, 85–86, 92–98
   PatternBrush.ts97.06%87.50%100%100%21
   PencilBrush.ts91.06%82.35%100%93.81%122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
   SprayBrush.ts0%0%0%0%107, 107, 107, 107, 107–108, 110–111, 118–119, 121, 123–127, 136, 140–141, 141, 148, 148, 148–151, 153–156, 160–161, 163, 165–168, 17, 171, 178–179, 18, 181, 183–184, 186, 193–194, 196–197, 20, 200, 200, 207, 207, 21, 211, 22, 22, 22–24, 28, 32, 39, 46, 53, 60, 67, 84–86, 94–96, 98–99
src/canvas
   Canvas.ts75.81%41.35%92.86%85.01%1004, 1007–1008, 1012–1013, 1018, 1063–1068, 1126, 1130, 1137, 1140, 1203–1207, 1286–1290, 1320, 1337, 1383–1400, 1406–1411, 1414–1415, 1417, 1421, 1423–1424, 1426–1428, 1432, 1434, 1436–1438, 1441–1446, 1449–1451, 1454, 1456, 1470, 1477, 1479–1490, 1492–1495, 1495, 1497, 1501–1502, 1505–1506, 1509–1511, 1514, 1522, 354, 369, 388, 443, 559–563, 566–567, 569, 579, 582–583, 585, 588–590, 602, 609–613, 615–620, 622–626, 659, 661, 668–672, 674–679, 681, 683–684, 686–689, 691–692, 747, 783–786, 789, 791, 794, 796, 798, 824, 887, 932–933, 936, 938, 948–953, 956, 963–964, 964, 966–970, 972, 974, 995–996, 999
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts85.49%53.85%100%94.43%1012, 1021, 1089–1093, 1139–1140, 1142–1143, 1145, 1172, 1174–1175, 1175, 1177, 1216, 1218, 1220, 1236, 1238, 1266, 1269, 1288, 1290, 1294, 1312–1319, 1322, 1325, 1334–1335, 1339–1348, 1352, 1354–1355, 1361–1366, 1370, 362, 384, 462, 513, 515, 591, 596, 673, 701, 992, 994–995
   StaticCanvas.ts75.88%70.19%98.91%75.36%1008, 1013, 1019, 1022, 1024, 1026, 1034, 1036–1042, 1052, 1055, 1058–1064, 1066–1067, 1069–1090, 1097–1099, 1101, 1109, 1115, 1117, 1117–1118, 1118–1119, 1121, 1121–1126, 1139, 1144–1146, 1150, 1154, 1156–1158, 1163–1167, 1169, 1171–1174, 1177, 1179, 1188, 1190–1191, 1198–1201, 1203, 1209–1212, 1216–1217, 1227, 1233–1235, 1237–1242, 1242, 1242–1246, 1246, 1246–1251, 1253–1260, 1289–1290, 1292–1293, 1295–1297, 1299–1305, 1309, 1311–1324, 1332–1333, 1343, 1354, 1395–1399, 1401, 1403, 1405–1409, 1428, 1444, 1461, 1481, 1514, 1518, 1529–1531, 438, 449,

@asturur asturur marked this pull request as ready for review June 3, 2024 23:22
@asturur asturur merged commit 8ddc7fe into master Jun 3, 2024
22 checks passed
@asturur asturur deleted the perf/regex-const branch June 3, 2024 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants