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

[Balearica]

CheckList

• [X] I agree to follow this project's Code of Conduct
• [X] I have read and followed the Contributing Guide
• [X] I have read and followed the Issue Tracker Guide
• [X] I have searched and referenced existing issues and discussions
• [X] I am filing a BUG report.
• [X] I have managed to reproduce the bug after upgrading to the latest version
• [X] I have created an accurate and minimal reproduction

Version

6.0.0-rc1

In What environments are you experiencing the problem?

Browser. Qualitatively similar results in Chrome and Firefox, on Windows and Linux desktops.

Node Version (if applicable)

None

Link To Reproduction

https://github.com/scribeocr/fabric.js-benchmark

Steps To Reproduce

Creating text objects (including Text and IText) appears to be noticeably slower upon updating from v5 to v6. I created a basic example, which adds several hundred IText objects to a canvas using both v5.3.0 and v6.0.0-rc1 multiple times, and prints the average time for each Fabric.js version to the console. It can be run by visiting this page, and the code is in this repo. On my system (Chrome, Windows), the average runtime is 128.84ms for v5 and 209.52ms for v6.

This increase is significant enough to be noticeable in applications that render many text boxes (which is how I noticed). I am hoping it is possible to restore performance on par with v5, as I think the type support in v6 makes it much easier to work with, so would like to eventually switch over.

Expected Behavior

Similar performance between v5 and v6.

Actual Behavior

See above.

Error Message & Stack Trace

No response

[Balearica]

I did some testing, and was able to identify the PR that caused the bulk of the performance regression: https://github.com/fabricjs/fabric.js/pull/8719

To demonstrate, I added a built version of Fabric.js using this commit and the previous commit to my benchmark site. Looking at the timings (printed to console), you will find that the 26ed225 version resembles the performance of v6.0.0-rc1, while the 196bea1 version resembles the performance of v5.3.0.

After running several tests, I believe the following code was the problematic addition: https://github.com/fabricjs/fabric.js/blob/26ed225b47288f775e598f719e218a660dd58dd0/src/shapes/Object/Object.ts#L285-L288

There appear to be 2 reasons this edit resulted in reduced performance.

1. The new method of assigning properties using Object.assign is slower than the method it replaced (inheritance with prototypes)
   1. I do not understand the nuances of Object.assign, however this block of code seems to speed up if switched to a for loop, so Object.assign seems to be uniquely slow.
2. The list of default properties now needs to be constructed every time an object is created, and getDefaults is non-trivial to compute.
   1. When getDefaults is called within an IText object, a total of 4 different getDefaults functions are called, getting the defaults for the following objects: IText, Text, InteractiveFabricObject, FabricObject
   2. Every call to a getDefaults function uses a spread operator to force the values to be copied, so a non-trivial amount of memory allocation and garbage collection happens every time this is run.

There are several ways this could be fixed, however this runs into some fundamental questions regarding how objects should work in v6, so I'm not going to open a PR for now. I believe any solution that (1) switches from Object.assign to something else and (2) does not re-calculate defaults for every new object (or re-calculates them in a much cheaper way) would solve.

[ShaMan123]

Awesome work!

[ShaMan123]

@jiayihu FYI

[jiayihu]

Nice findings. I don't think that Object.assign matters that much, especially if it's not compiled to ES5 but to native ES6: http://incaseofstairs.com/six-speed/ I do think however that the dynamic getDefaults resolution matters. In my own profiling, I think I saw once that for Texts this piece is problematic:

static getDefaults(): Record<string, any> {
    return {
      ...super.getDefaults(),
      controls: createTextboxDefaultControls(),
      ...Textbox.ownDefaults,
    };
}

createTextboxDefaultControls is called each time and instantiating+copying the Controls was not cheap:

const createTextboxDefaultControls = () => ({
  ...createObjectDefaultControls(),
  ...createResizeControls(),
});

@Balearica if you can create a CodeSandbox example from your repo it would be great, so that we can easily spin it up and measure performance with Chrome Devtools.

[asturur]

@Balearica thank you for opening this issue.

I want to clarify that i know v6 is slower in a number of ways and i knew this was one.

You have to consider that now for every object we create we instantiate 9 new controls + override the 9 controls of the default object. So Text creates 18 controls

This was one of the reason why i wanted to stay on function + prototype in general, because it would have been faster, classes has been chosen because typescript can't handle function + prototype and because modern code.

And this is also one of the reason why i m angry when i m answered 'it does not matter' or 'it is modern code' or 'it is best practice' at this level of complexity every change is a bet, and the reason why there will be less and less easy changes.

The first thing you could do is control creation from the prototype and work with shared controls as v5 was doing.

[asturur]

Anyway on my machine if i slow down the cpu to see better, on 40 ms of constructor cost ( slowed down ) only 4 are spent on default properties:

The rest 90% is spent on initDimensions, and this in an example without actual text, an empty string.

[jiayihu]

on 40 ms of constructor cost ( slowed down ) only 4 are spent on default properties:

That is consistent wih the findings I remember, i.e. a 10% improvement, so a marginal improvement but not game changing.

This was one of the reason why i wanted to stay on function + prototype in general, because it would have been faster, classes has been chosen because typescript can't handle function + prototype and because modern code.

Pixi also uses classes so I don't think that's an issue, maybe at most fabric is using too much inheritance / hierarchy. Also the previous custom subclassing implementation in fabric was a nightmare to debug, notably following super calls as the debugger would continously jump into the custom subclassing. Then there were a few bugs I discovered with the old method resolution that were a nightmare.

[asturur]

yes for example callSuper couldn't have an empty jump. Indeed i started to call them manually with call method because callSuper couldn't work

[asturur]

@Balearica since you have the benchmark setup i would suggest instead of using 4 version, use v5 vs v6 with controls and default values on the prototype, that will rule out if that is the difference.

for every class from IText up ( since you are using IText) that has a getDefaults method do the following:

IText.getDefaults = () => {}
// for each class in the chain that has a ownDefaults object:
Object.assign(IText.prototype, Textbox.ownDefaults);
Object.assign(Text.prototype, IText.ownDefaults);
Object.assign(FabricObject.prototype, FabricObject.ownDefaults);

and check if the situation moves back to what you expect. if it doesn't the issue is somewhere else.

[Balearica]

on 40 ms of constructor cost ( slowed down ) only 4 are spent on default properties: The rest 90% is spent on initDimensions, and this in an example without actual text, an empty string.

That is consistent wih the findings I remember, i.e. a 10% improvement, so a marginal improvement but not game changing.

I'm not sure what code was run for this, however if initDimensions is the bulk of the runtime, I believe that would only happen with if creating one IText objects (or a small number). The first time an IText object is created, the bulk of the runtime is indeed attributable to initDimensions, as calculating the text metrics is an expensive operation. However, the metrics are cached, so when creating a large number of IText objects, the runtime attributable to initDimensions drops considerably.

Using Chrome devools in the example site shows that most calls to initDimensions take ~1ms in all versions of Fabric.js, even after enabling 6x CPU slowdown, and the "Bottom-Up" view shows that this function is a relatively small proportion of total runtime.

Reviewing the other messages now.

[Balearica]

@Balearica since you have the benchmark setup i would suggest instead of using 4 version, use v5 vs v6 with controls and default values on the prototype, that will rule out if that is the difference.

for every class from IText up ( since you are using IText) that has a getDefaults method do the following:

IText.getDefaults = () => {}
// for each class in the chain that has a ownDefaults object:
Object.assign(IText.prototype, Textbox.ownDefaults);
Object.assign(Text.prototype, IText.ownDefaults);
Object.assign(FabricObject.prototype, FabricObject.ownDefaults);

@asturur I implemented this and added the built version to the benchmark page. The code can be seen in PR https://github.com/fabricjs/fabric.js/pull/9862. Making this change eliminates the vast majority of the performance gap between v5 and v6. On my system the timings were as follows:

• v5.3.0 - 122ms
• v6 - 203ms
• v6 + edits - 139ms

Therefore, I believe we can conclude that this is definitely the root cause of the performance regression, and reverting to assigning defaults using prototypes would be one way of restoring the performance delivered with v5.

[asturur]

That is not something that we can rollback, it was a decision taken with discussions to move in that directions. I'm not sure if the perf issue is because controls or values or the way the functions are built, i also have difficulties understanding why you get 139 vs 203 ms is a lot, is 64 of difference and i m not sure what part the benchmark is measuring.

The code i gave you is also removing the controls creation all at once, i m not sure if that is the issue, we should find a way to understand which part cost how much and see what tweaks can be done.

Removing the getDefault functions means that you can't configure the instances outside runtime, if you want to have a different default value for fabric you have to change it instance by instance, using the the class public properties means that the changes to prototype will not have effect and you don't have a way to rollback to it.

This is a middle ground if we can tweak it, great.

[Balearica]

i also have difficulties understanding why you get 139 vs 203 ms is a lot, is 64 of difference and i m not sure what part the benchmark is measuring.

The benchmark measures the entire time to create the objects and render the page, as it is intended to show the full time to create and render new contents to a Fabric.js canvas. The source code is here.

I maintain a website that renders canvases that include many objects in response to user input. The lag between the user pressing the button and the canvas being rendered increased from ~0.1s (122ms) to ~0.2s (203ms) when I tried updating Fabric.js from v5 to v6. This added lag was enough to make the application feel noticeably more sluggish.

Removing the getDefault functions means that you can't configure the instances outside runtime, if you want to have a different default value for fabric you have to change it instance by instance, using the the class public properties means that the changes to prototype will not have effect and you don't have a way to rollback to it.

Can you explain this further? Specifically, what can developers achieve by modifying ownDefaults that can not be achieved by modifying object prototypes? I can try and think of a solution that makes everybody happy, however I'm still not sure I understand why modifying the prototype (as was done through v5) was determined to be inadequate.

[asturur]

The issue with object properties is that in the past if you didn't want to deal with strokeWidth you would do:

FabricObject.prototype.strokeWidth = 0;

and every object would be created by default with that strokeWidth. If you want all text to be Arial you would do similarly with fontFamily.

With classes you have to create a subclass to do so, and if you want to change something that is in the base obejct you have to subclass all the objects that you intend to use.

Or you have to change the object constructor to assign the property as you want risking that then you go out of sync with updates.

With javascript classes declaration there are no values on the prototype, just methods. On the prototype itself you can add values and you inheritance works.

[asturur]

Other things you can try, sorry if i just leave you tasks, but this is all i can do right now:

remove this spread here:

  static getDefaults(): Record<string, any> {
    return { ...FabricObject.ownDefaults };
  }

and just return FabricObject.ownDefaults as it is.

then another thing you can do is to just remove the controls creation:

// InteractiveObject.ts
  static getDefaults(): Record<string, any> {
    return {
      ...super.getDefaults(),
      controls: createObjectDefaultControls(), // <--- remove this
      ...InteractiveFabricObject.ownDefaults,
    };
  }

// textbox.ts
  static getDefaults(): Record<string, any> {
    return {
      ...super.getDefaults(),
      controls: createTextboxDefaultControls(), // <--- remove this
      ...Textbox.ownDefaults,
    };
  }

by doing

Textbox.getDefaults = function() {
    return {
      ...super.getDefaults(),
      ...Textbox.ownDefaults,
    };
}

Textbox.prototype.controls = createTextboxDefaultControls();

// hopefully this is reacheable thorugh FabricObject :( or try to get your way there by looking at prototype.prototype
InteractiveFabricObject.getDefaults = function() {
    return {
      ...super.getDefaults(),
      ...InteractiveFabricObject.ownDefaults,
    };
}

FabricObject.prototype.controls = createObjectDefaultControls();

If you can't do it at runtime make your branch and give it a check, if removing the first spread that is the larges and the controls make a difference we can remove the spread and make sure controls can be deactivated and put on the prototype, that is a feature that needs to be documented anyway, we just need to make sure we didn't unintentionally make it impossible

[Balearica]

The issue with object properties is that in the past if you didn't want to deal with strokeWidth you would do:

FabricObject.prototype.strokeWidth = 0;

and every object would be created by default with that strokeWidth. If you want all text to be Arial you would do similarly with fontFamily.

With classes you have to create a subclass to do so, and if you want to change something that is in the base obejct you have to subclass all the objects that you intend to use.

Or you have to change the object constructor to assign the property as you want risking that then you go out of sync with updates.

With javascript classes declaration there are no values on the prototype, just methods. On the prototype itself you can add values and you inheritance works.

I understand that switching to assigning the values in the constructor facilitates these changes, however am trying to understand what the motivation behind this switch was, so I can keep those in mind when making suggestions. Was the change from assigning defaults through prototypes to assigning in the constructor made for purely aesthetic and/or type inference purposes, or does it impact the capabilities of Fabric.js in some way?

To make the question more concrete: say that I want to change the default font from Times New Roman to Arial. In Fabric.js v5, you would run the following code:

fabric.Text.prototype.fontFamily = 'Arial'

In the current master branch, you would run the following code:

fabric.FabricText.ownDefaults.fontFamily = 'Arial';

In the version of v6 with defaults stored on prototypes (the contents of PR https://github.com/fabricjs/fabric.js/pull/9862), you would do the following:

fabric.FabricText.prototype.fontFamily = 'Arial';

What are the downsides of the prototype.fontFamily implementation versus the ownDefaults.fontFamily implementation?

Other things you can try, sorry if i just leave you tasks, but this is all i can do right now:

remove this spread here:

The suggested change to createObjectDefaultControls reduces runtime in the benchmark by ~8ms; removing the spread operator for FabricObject.ownDefaults reduces runtime by ~6ms.

I think the spread operators in the getDefaults functions are definitely the core issue, however simply removing this single spread operator does not move the needle much, as every other getDefaults function between IText and FabricObject still re-assigns the defaults using the spread operator.

[asturur]

The changes was made because collaborators said they were unhappy to work in a codebase that made change to the prototype, because considered hacky. We debated and landed on agreement we would remove it. The change is functional in the part i described, so that with classes there are no values on the prototype by syntax, you have to add them manually on the prototype.

The slight differences are in ability to configure defaults and ability to change defaults at runtime.

In this case the spread is repeated for each subclass that has the method, in the case of IText those are:

• FabricObject
• InteractiveObject
• Text
• IText

So that large object grows each subclass and gets respread every time. Maybe we can just collect the objects and spread them all at once? that would mean changing the return signature of getDefaults

[asturur]

Any solution that reduces the amount of spread and copy will help. The rules are:

• defaults of Text override defaults of interactiveObject, and the one of Itext override Text.
• passed options override everything

The one i can think of is:

• each constructor assign its own defaults and the options
• we stop passing up the options to super

is very late so i may be completely wrong

[asturur]

@Balearica i ll work on this tomorrow. Did you do more experiements on your side?

[Balearica]

@asturur I don't have much to add. One "quick fix" I tried was caching the default options for IText the first time they are calculated, and then applying those default options directly in the IText constructor from then on. While this does fix the performance issue, it also prevents editing default options after the first IText object is created, so I don't think that would work for the main project.

[shaktippatel]

@Balearica

Could you provide your code, if feasible?

[asturur]

I have a working branch for this, i had to rewrite all the constructors and a bunch of things, i also uncovered some weird inconsistencies on how we apply values, i ll push it up for a test when is ready.

[Balearica]

@shaktippatel It sounds like an official solution is being developed. If you want ideas for a quick fix in the meantime, I would take a look at the branch I made that reverts to using prototypes, which can be found here. I think that works better than the extremely simplistic/half-baked implementation of caching described in my previous comment, which does not allow for editing object defaults after the first object is created. That code is below, which comes from editing the Objects.ts file.

  static defaultsCache: Record<string, any> = {}

  /**
   * Constructor
   * @param {Object} [options] Options object
   */
  constructor(options: Props = {} as Props) {
    super();

    if (!FabricObject.defaultsCache[this.constructor.name]) {
      FabricObject.defaultsCache[this.constructor.name] = (this.constructor as typeof FabricObject).getDefaults();
    }

    Object.assign(
      this,
      FabricObject.defaultsCache[this.constructor.name]
    );
    this.setOptions(options);
  }

[asturur]

@Balearica https://github.com/fabricjs/fabric.js/pull/9891 i have still to understand how to add controls here and keep overreadibility but this is what i was trying. i didn't benchmark it yet.

[asturur]

[Balearica]

Thanks for figuring this out. Looks like performance is now similar to v5.