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

DOMTokenList: Reuse index from DOMTokenList instance if possible … #166

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

amakhrov
Copy link
Contributor

@amakhrov amakhrov commented Jun 2, 2020

…instead of parsing raw string attribute every time

In our Angular Universal app we extensively use utility classes that can be toggled conditionally.
While investigation recent performance issues, I noticed that the single longest-running piece of code is getList function from DOMTokenList. Looks like adding multiple classes to a dom element one by one is O(N^2), as at each step getList is called and parses the raw string property into a list of tokens. This parsing seems to be quite a heavy operation.

This PR implements a way to avoid unnecessary parsing by reusing results from the previous step.

In our project this change reduced the class list processing time by half.

…ead of parsing raw string attribute every time
@@ -156,8 +157,21 @@ function handleErrors(token) {
return token;
}

function toArray(clist) {
var length = clist._length;
var arr = Array(length);

Choose a reason for hiding this comment

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

Nice 😊

@alan-agius4
Copy link

@cscott, can you please take a look at this? This PR is really important for the Angular team.
Thanks.

@cscott
Copy link
Collaborator

cscott commented Jul 16, 2020

This doesn't actually fix the O(N^2) behavior. You'd need to keep the hash of elements from getList() around so that the uniqueness check in add is O(1) in order to avoid O(N^2) behavior. But really you probably don't want the # of elements in the class string to be very large in the first place, so I'm guessing the O(N^2) aspect isn't what you're actually concerned with, but rather the constant factor. This patch looks fine for reducing the constant factor.

@cscott cscott merged commit 571453c into fgnass:master Jul 16, 2020
cscott added a commit that referenced this pull request Jul 16, 2020
@amakhrov
Copy link
Contributor Author

Good observation @cscott ! This is still N^2 with a much smaller constant.
The reason I chose not to cache the parsed array (which would be the ultimate solution) is to be more memory-efficient. For some consumers memory might be more important than cpu, and I just didn't want to make this decision :)

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.

3 participants