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(): Rework constructors to avoid the extra perf cost of current setup #9891

Merged
merged 13 commits into from
May 30, 2024

Conversation

asturur
Copy link
Member

@asturur asturur commented May 25, 2024

Description

close #9852

There were (at least) 2 problems with the v6 object initialization.
getDefaults was recursive running up from leaf to root class, spreading the result each time.

Now Textbox calls Itext, then FabricText, then InteractiveObject, then FabricObject getDefaults.

The Object getDefaults() was the largest object of the all, spread once in its own function but then spread again every time it returned from InteractiveObject, FabricText, IText, TextBox so in the worst case there were 5 unnecessary spread operations.

the function getDefaults() has to stay because is necessary for exporting object and removing default values.

What changes now is that every constructor has the duty to assign its own options and defaults, to avoid assigning options more than once we stop passing options to the super() call.

Text / IText / Textbox are its own particular case because they need to assign all the defaults and options before doing the measurements. So in this case we are still spreading options together with default values, this could make worse restoring an object compared as before, but i don't really have a solution for this. In thise case the defaults value are small objects so hopefully all in all is still overall better performance compared to before.

The best option performance wise would be adding a third parameter to the constructor to carry defaults separated from options, but until that is necessary i prefer to not do it

In Action

Copy link

codesandbox bot commented May 25, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link
Contributor

github-actions bot commented May 25, 2024

Build Stats

file / KB (diff) bundled minified
fabric 918.963 (+1.632) 306.235 (+0.604)

@asturur
Copy link
Member Author

asturur commented May 26, 2024

I could offset the added code but i can't really get rid of getDefaults() because that function is still necessary for the 'includeDefaults' functionality during export.

* Constructor
* @param {String} text Text string
* @param {Object} [options] Options object
*/
constructor(text: string, options?: Props) {
super(text, options);
super(text, { ...IText.ownDefaults, ...options } as Props);
Copy link
Member Author

Choose a reason for hiding this comment

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

Can't avoid rebuilding the object for Textbox -> IText -> Text because of how text measuring works. All the defaults needs to be assigned before Text base class execute the measurement of the text.

@asturur asturur marked this pull request as ready for review May 26, 2024 08:09
@asturur
Copy link
Member Author

asturur commented May 26, 2024

There are 2 failing unit tests, hopefully are just a small undersight and not a fundamental issue

@asturur
Copy link
Member Author

asturur commented May 26, 2024

with this branch i get
for 100.000 Textbox between 2.8 to 3.1 seconds around 25% of current master
for 100.000 Polygons between 0.81 to 0.84 seconds around 17% of current master
for 100.000 Rects around 0.7 seconds around 17% of current master

With master:
for 100.000 Textbox around 12 seconds
for 100.000 Polygons between 4.9 to 5.05 seconds
for 100.000 Rects between 4.1 to 4.2 seconds

This happens in a tight loop with no pauses so not really a real use case but still numbers are definitely better especially for simpler classes.

@asturur
Copy link
Member Author

asturur commented May 26, 2024

If i try to replicate the same on fabric 5.x in jsfiddle, this is what i get:

for 100.000 Textbox around 0.75 seconds
for 100.000 Polygons between 15 millisecond
for 100.000 Rects between 10 milliseconds

So we are still miles away from the original performance
I can go down to 60ms for the Rect in fabric 6, if i turn back to the values on prototype, still 6 time slower than original, but that may be related to a series of things, code changed, more properties, classes vs functions, all things i won't be able to benchmark in a syntethic way.

@Balearica i would appreciate if you could run this branch in your benchmark to see a real use case

@asturur
Copy link
Member Author

asturur commented May 26, 2024

I made some dumb test here and there , producing some non real-life test case
I create a bunch of classes like this:

class a {
  constructor() {
    Object.assign(this, fabricObjectDefaultValues);
  }

  method1() {
    return 1;
  }

  method2() {
    return 1;
  }

  method3() {
    return 1;
  }

  method4() {
    return 1;
  }

  method5() {
    return 1;
  }

  method6() {
    return 1;
  }

  method7() {
    return 1;
  }

  method8() {
    return 1;
  }
}

class b extends a {
  constructor() {
    super();
    Object.assign(this, textDefaultValues);
  }

  method7() {
    return 1;
  }

  method8() {
    return 1;
  }

  method9() {
    return 1;
  }
}

export class c extends b {
  method10() {
    return 1;
  }
}

class a1 {
  method1() {
    return 1;
  }

  method2() {
    return 1;
  }

  method3() {
    return 1;
  }

  method4() {
    return 1;
  }

  method5() {
    return 1;
  }

  method6() {
    return 1;
  }

  method7() {
    return 1;
  }

  method8() {
    return 1;
  }
}

Object.assign(a1.prototype, fabricObjectDefaultValues);

class b1 extends a1 {
  constructor() {
    super();
  }

  method7() {
    return 1;
  }

  method8() {
    return 1;
  }

  method9() {
    return 1;
  }
}

Object.assign(b1.prototype, textDefaultValues);

export class c1 extends b1 {
  method10() {
    return 1;
  }
}

class a2 {
  top = 0;
  left = 0;
  width = 0;
  height = 0;
  angle = 0;
  flipX = false;
  flipY = false;
  scaleX = 1;
  scaleY = 1;
  minScaleLimit = 0;
  skewX = 0;
  skewY = 0;
  originX = 'LEFT';
  originY = 'TOP';
  strokeWidth = 1;
  strokeUniform = false;
  padding = 0;
  opacity = 1;
  paintFirst = 'fill';
  fill = 'rgb(0,0,0)';
  fillRule = 'nonzero';
  stroke = null;
  strokeDashArray = null;
  strokeDashOffset = 0;
  strokeLineCap = 'butt';
  strokeLineJoin = 'miter';
  strokeMiterLimit = 4;
  globalCompositeOperation = 'source-over';
  backgroundColor = '';
  shadow = null;
  visible = true;
  includeDefaultValues = true;
  excludeFromExport = false;
  objectCaching = true;
  clipPath = undefined;
  inverted = false;
  absolutePositioned = false;
  centeredRotation = true;
  centeredScaling = false;
  dirty = true;

  method1() {
    return 1;
  }

  method2() {
    return 1;
  }

  method3() {
    return 1;
  }

  method4() {
    return 1;
  }

  method5() {
    return 1;
  }

  method6() {
    return 1;
  }

  method7() {
    return 1;
  }

  method8() {
    return 1;
  }
}

class b2 extends a2 {
  fontSize = 40;
  fontWeight = 'normal';
  fontFamily = 'Times New Roman';
  underline = false;
  overline = false;
  linethrough = false;
  textAlign = 'left';
  fontStyle = 'normal';
  lineHeight = 1.16;
  superscript = {
    size: 0.6, // fontSize factor
    baseline: -0.35, // baseline-shift factor (upwards)
  };
  subscript = {
    size: 0.6, // fontSize factor
    baseline: 0.11, // baseline-shift factor (downwards)
  };
  textBackgroundColor = '';
  stroke = null;
  shadow = null;
  path = undefined;
  pathStartOffset = 0;
  pathSide = 'left';
  pathAlign = 'baseline';
  _fontSizeFraction = 0.222;
  offsets = {
    underline: 0.1,
    linethrough: -0.315,
    overline: -0.88,
  };
  _fontSizeMult = 1.13;
  charSpacing = 0;
  deltaY = 0;
  direction = 'ltr';
  CACHE_FONT_SIZE = 400;
  MIN_TEXT_WIDTH = 2;

  constructor() {
    super();
  }

  method7() {
    return 1;
  }

  method8() {
    return 1;
  }

  method9() {
    return 1;
  }
}

export class c2 extends b2 {
  method10() {
    return 1;
  }
}

function a3() {}

Object.assign(a3.prototype, {
  top: 0,
  left: 0,
  width: 0,
  height: 0,
  angle: 0,
  flipX: false,
  flipY: false,
  scaleX: 1,
  scaleY: 1,
  minScaleLimit: 0,
  skewX: 0,
  skewY: 0,
  originX: 'LEFT',
  originY: 'TOP',
  strokeWidth: 1,
  strokeUniform: false,
  padding: 0,
  opacity: 1,
  paintFirst: 'fill',
  fill: 'rgb(0,0,0)',
  fillRule: 'nonzero',
  stroke: null,
  strokeDashArray: null,
  strokeDashOffset: 0,
  strokeLineCap: 'butt',
  strokeLineJoin: 'miter',
  strokeMiterLimit: 4,
  globalCompositeOperation: 'source-over',
  backgroundColor: '',
  shadow: null,
  visible: true,
  includeDefaultValues: true,
  excludeFromExport: false,
  objectCaching: true,
  clipPath: undefined,
  inverted: false,
  absolutePositioned: false,
  centeredRotation: true,
  centeredScaling: false,
  dirty: true,
  method1() {
    return 1;
  },

  method2() {
    return 1;
  },

  method3() {
    return 1;
  },

  method4() {
    return 1;
  },

  method5() {
    return 1;
  },

  method6() {
    return 1;
  },

  method7() {
    return 1;
  },

  method8() {
    return 1;
  },
});

function b3() {}

b3.prototype = new a3();
b3.prototype.constructor = b3;
Object.assign(b3.prototype, {
  method7() {
    return 1;
  },

  method8() {
    return 1;
  },

  method9() {
    return 1;
  },
  fontSize: 40,
  fontWeight: 'normal',
  fontFamily: 'Times New Roman',
  underline: false,
  overline: false,
  linethrough: false,
  textAlign: 'LEFT',
  fontStyle: 'normal',
  lineHeight: 1.16,
  superscript: {
    size: 0.6, // fontSize factor
    baseline: -0.35, // baseline-shift factor (upwards)
  },
  subscript: {
    size: 0.6, // fontSize factor
    baseline: 0.11, // baseline-shift factor (downwards)
  },
  textBackgroundColor: '',
  stroke: null,
  shadow: null,
  path: undefined,
  pathStartOffset: 0,
  pathSide: 'LEFT',
  pathAlign: 'baseline',
  _fontSizeFraction: 0.222,
  offsets: {
    underline: 0.1,
    linethrough: -0.315,
    overline: -0.88,
  },
  _fontSizeMult: 1.13,
  charSpacing: 0,
  deltaY: 0,
  direction: 'ltr',
  CACHE_FONT_SIZE: 400,
  MIN_TEXT_WIDTH: 2,
});

export function c3() {}

c3.prototype = new b3();
c3.prototype.constructor = b3;

Object.assign(c3.prototype, {
  method10() {
    return 1;
  },
});

And then benched them 100000 time
image

So if there is any work going on for the values with object assign or public properties, the construction time is completely different. If you want to have them super fast you have to move the values in the prototype. period.
The longer time we see for fabric6 vs fabric5 when we move the values in the prototype are probably related to the multiple empty Object.assign calls and empty setOptions

@jiayihu
Copy link
Contributor

jiayihu commented May 26, 2024

Just to throw an idea, what if ClassRegistry.setClass() calls getDefaults() and assign it to the prototype for you? So it's done only once, it's on the prototype but without requiring the dev to do it manually.

Otherwise honestly I think it's okay for people to manually do MyTextbox.prototype.defaults = { ...fabric.Textbox.getDefaults(), align: "right" }. You do it once and never get bothered with it. The only issue is with controls, but it can be workarounded as well.

@asturur
Copy link
Member Author

asturur commented May 27, 2024

The team chose to keep value per class. I wasn't excited but that is it, i m not rolling back that now.
I wanted to leave a workaround and that is why defaults are stored in a configurable object, I just need to add a detailed page that explains pro and cons to work in a way or another.

@asturur
Copy link
Member Author

asturur commented May 27, 2024

This PR a a side effect makes the workaround easier

Copy link
Contributor

Coverage after merging constructor-performace into master will be

84.51%

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.ts87.85%78.26%100%88.37%147, 150, 152–157, 166, 203, 206, 213, 230–237, 241–242, 38–41
   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.49%39.76%92.86%84.92%1004, 1007–1008, 1012–1013, 1018, 1063–1068, 1105, 1126, 1128, 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, 886–887, 932–933, 935–936, 938, 947–953, 956, 963–964, 966–970, 972, 974, 995–996, 999
   CanvasOptions.ts100%100%100%100%
   SelectableCanvas.ts85.35%53.54%100%94.34%1012, 1021, 1089–1093, 1139–1140, 1142–1143, 1145, 1172, 1174–1175, 1175, 1177, 1216, 1218, 1220, 1236, 1238, 1242, 1245, 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.93%70.11%98.91%75.43%1003, 1009, 1012, 1014, 1016, 1024, 1026–1032, 1042, 1045, 1048–1054, 1056–1057, 1059–1080, 1087–1089, 1091, 1099, 1105, 1107, 1107–1108, 1108–1109, 1111, 1111–1116, 1129, 1134–1136, 1140, 1144, 1146, 1148, 1153–1157, 1159, 1161–1164, 1167, 1169, 1178, 1180–1181, 1188–1191, 1193, 1199–1202, 1206–1207, 1217, 1223–1225, 1227–1232, 1232, 1232–1236, 1236, 1236–1241, 1243–1250, 1279–1280, 1282–1283, 1285–1287, 1289–1295, 1299, 1301–1314, 1322–1323, 1333, 1344, 1385–1389, 1391, 1393, 1395–1399, 1418, 1433, 1448, 1468, 1501,

@asturur
Copy link
Member Author

asturur commented May 30, 2024

Using the benchmark provided by balerica i can get here with the default values

image

@asturur
Copy link
Member Author

asturur commented May 30, 2024

Moving back controls to the prototype with a simple line goes here

image

this is the code to move controls out

Object.assign(fabric6Latest.FabricObject.prototype, fabric6Latest.FabricObject.createControls());

fabric6Latest.FabricObject.createControls = function () {
    return {};
};

@asturur
Copy link
Member Author

asturur commented May 30, 2024

Moving also away from all the defaults per instance goes back to v5 level, with a difference of 1-2 ms, so definitely ok.

@asturur asturur merged commit 1025c81 into master May 30, 2024
22 checks passed
@asturur asturur deleted the constructor-performace branch May 30, 2024 16:25
@asturur
Copy link
Member Author

asturur commented May 30, 2024

Hopefully i didn't break anything shady.

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.

[Bug]: Creating Text/IText Objects Significantly Slower in v6 vs. v5
2 participants