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.

Sign up for GitHub

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

Jump to bottom

Widget: Optimize attachment of the _untrackClassesElement listener #2037

Merged
merged 1 commit into from
Jan 15, 2022
Merged

Widget: Optimize attachment of the _untrackClassesElement listener #2037

merged 1 commit into from
Jan 15, 2022

Conversation

mgol
Copy link
Member

@mgol mgol commented Dec 31, 2021

jQuery UI 1.13.0 changed the logic attaching the _untrackClassesElement
listener in the _classes widget method; one of the side effects was calling
this._on for each node that needed the listener. That caused a severe
performance degradation for large comboboxes as each _on jQuery UI call
causes a jQuery add call that calls Sizzle's uniqueSort underneath.

Instead, collect the nodes that need the listener and then, outside of the loop,
create a jQuery object out of them and attach the listener once. That's still
slower than the jQuery 1.12 version but only slightly: 936 ms to 1.03s on a very
large list on a recent MacBook Pro, compared to ~30 seconds before this patch.

Fixes gh-2014

@mgol mgol added this to the 1.13.1 milestone Dec 31, 2021
@mgol mgol requested a review from fnagel December 31, 2021 19:05
@mgol mgol self-assigned this Dec 31, 2021
@mgol mgol mentioned this pull request Dec 31, 2021
Closed
@mgol
Copy link
Member Author

mgol commented Dec 31, 2021

I'm not sure how to verify whether this doesn't break Scott's fixes from ef2e9ba as that patch didn't add any test. @fnagel would you be able to test it a bit?

@fnagel
Copy link
Member

fnagel commented Jan 3, 2022 •
edited
Loading

I've checked the bugs Scott fixed and here are what the bugs are about:

  • Using autocomplete over and over again
  • Have a large collection of items in autocomplete
  • Using refresh method on selectmenu over and over again
  • Adding lots of new tabs to the tabs widget

Some more references for the commit (ef2e9ba) that introduced the bug this PR fixes:

fnagel
fnagel approved these changes Jan 3, 2022
View reviewed changes
Copy link
Member

@fnagel fnagel left a comment

Choose a reason for hiding this comment

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

Code and its logic loos good to me and makes sense.

@fnagel
Copy link
Member

fnagel commented Jan 3, 2022 •
edited
Loading

I can do some manual testing. It is too bad there are no tests for this, but what about adding some now? Or is it tricky to test for a memory leak / performance issue?

@mgol
Copy link
Member Author

mgol commented Jan 5, 2022

@fnagel I guess we could add a test for some of these, e.g. https://bugs.jqueryui.com/ticket/15082 as we can access our event handlers list and see if it doesn't grow too much. I'll think about possible tests next week.

@mgol mgol force-pushed the combobox-perf-fix-gh-2014 branch from 4d2bd24 to cd0fe70 Compare January 5, 2022 11:12
@fnagel
Copy link
Member

fnagel commented Jan 9, 2022

Wonderful. I will do some more testing when you finished.

@mgol
Copy link
Member Author

mgol commented Jan 13, 2022

@fnagel I added tests for all of the issues except for https://bugs.jqueryui.com/ticket/15136 but that one will be almost identical to the existing tests so it should be good for you to test now.

I verified those new tests fail when Scott's change is reverted and - as you can see - they pass on my branch.

@mgol
Widget: Optimize attachment of the _untrackClassesElement listener
7a951ef 
jQuery UI 1.13.0 changed the logic attaching the `_untrackClassesElement`
listener in the `_classes` widget method; one of the side effects was calling
`this._on` for each node that needed the listener. That caused a severe
performance degradation for large comboboxes as each `_on` jQuery UI call
causes a jQuery `add` call that calls Sizzle's `uniqueSort` underneath.

Instead, collect the nodes that need the listener and then, outside of the loop,
create a jQuery object out of them and attach the listener once. That's still
slower than the jQuery 1.12 version but only slightly: 936 ms to 1.03s on a very
large list on a recent MacBook Pro, compared to ~30 seconds before this patch.

Fixes jquerygh-2014
@mgol
Copy link
Member Author

mgol commented Jan 13, 2022

@fnagel I added a test case for the tabs leak as well. As for other tests, it fails if I revert Scott's patch and succeeds on my branch.

This should be ready for your final testing.

@fnagel fnagel self-requested a review January 14, 2022 22:25
fnagel
fnagel approved these changes Jan 14, 2022
View reviewed changes
Copy link
Member

@fnagel fnagel left a comment

Choose a reason for hiding this comment

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

Tested locally (reviewed in IDE, running tests and manual testing incl. the demo from issue). Good to merge!

@mgol mgol merged commit 0c5becc into jquery:main Jan 15, 2022
@mgol mgol deleted the combobox-perf-fix-gh-2014 branch January 15, 2022 00:26
@mgol mgol mentioned this pull request Jan 15, 2022
Closed
@mgol
Copy link
Member Author

mgol commented Jan 20, 2022

jQuery UI 1.13.1 including this fix has been released: https://blog.jqueryui.com/2022/01/jquery-ui-1-13-1-released/.

@fnagel fnagel mentioned this pull request Apr 12, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fnagel fnagel fnagel approved these changes

Assignees

@mgol mgol

Labels
Behavior different from 1.12 Bug Comp: Widget
Milestone
1.13.1
Development

Successfully merging this pull request may close these issues.

jquery autocomplete custom combobox- ui hangs
2 participants
@mgol @fnagel