-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Speed up of Chosen.winnow_results() method #522
Conversation
@@ -1295,7 +1295,7 @@ | |||
</ol> | |||
|
|||
</div> | |||
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.6.4/jquery.min.js" type="text/javascript"></script> | |||
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.3.2/jquery.min.js" type="text/javascript"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why using an old version for the example ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated jQuery example page according to the minimum supported version of the library.
the speed improvement should be implemented in the Prototype version too according to the contribution rules. |
This speed improvement has already been implemented in the Prototype version. |
|
||
for option in @results_data | ||
if option.html | ||
@trie.add(option.html, option.options_index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is duplicated in the Prototype and jQuery versions. couldn't it be moved to AbstractChosen ?
I moved Trie initialization to AbstractChosen.build_trie() method |
Thanks @asp24 and sorry for going so long without weighing in on this. A couple of days ago, I opened #1339 which makes Chosen significantly faster by replacing individual option dom manipulations with one big search results dom manipulation. There are are still performance gains to be made, but I believe that branch is going to land this week and this PR will have diverged too much for merge consideration. Your Infix Trie class comes with a lot of interesting ideas and I'll definitely be revisiting them. I've been loathe to add things like alternate search terms for options because performance has been such an issue. Perhaps this will help us get there. Thanks again! |
This patch adds support for jQuery >= 1.3.2 and considerably (up to 20 times) increases speed of the results window rendering by removing DOM search calls and caching properties of the most used elements (like 'active-result' class checking or setting of the result container content).