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

WIP: Move default values to prototype to resolve v6 performance regression #9862

Closed
wants to merge 1 commit into from

Conversation

Balearica
Copy link

Description

This is currently a proof of concept that should not be merged, but opening for visibility.

See #9852 for full context. In short, creating a large number of objects in v6 is significantly slower than doing so in v5. The root cause is the switch from assigning default values using object prototypes to setting defaults using getDefaults and Object.assign.

The relevant PR is here: #8719
The relevant block of code is here:

Object.assign(
this,
(this.constructor as typeof FabricObject).getDefaults()
);

This PR implements the quick fix described in this comment, which essentially rolls back the change shown above.

  1. The IText.getDefaults function, and all getDefaults functions it calls, are edited to return an empty object.
  2. The properties that would have been returned by these getDefaults functions are assigned to the object prototypes instead.

This is currently a proof of concept--the final implementation obviously shouldn't have a bunch of getDefaults functions that do nothing.

In Action

The improvement can be confirmed by visiting this web page containing a benchmark, and opening up the console (source code here). The relevant comparison is v6 (the master branch) and v6 edited (this PR). On my system the following is printed:

Mean time v6: 205.18 ms vs. v6 edited: 138.3 ms.

Copy link

codesandbox bot commented May 7, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@asturur
Copy link
Member

asturur commented May 7, 2024

@Balearica as mentioned in the discussion we cant do this, this is consider 'hacky' by many and also makes also hard to keep track of types and everything. We are happy to search a solution for faster text constructor time but i don't think will be this

@asturur
Copy link
Member

asturur commented May 27, 2024

close in favor of #9891

@asturur asturur closed this May 27, 2024
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

2 participants