[patch] Faster SelectBox for ManyToMany

[gkelly@…]

I'm attaching a patch to SelectBox.js which uses offline DOM manipulation to optimize SelectBox.redisplay(), which I found to be extremely slow for a list with more than 4,000 items. I'm getting around 75% faster execution; I'm also attaching some screenshots of the javascript profiler in ​FireBug as evidence.

[anonymous]

Patch to SelectBox.js

[anonymous]

Javascript profiler for original SelectBox.js

[anonymous]

Javascript profiler for faster SelectBox.js

[Gary Wilson <gary.wilson@…>]

see also #3099.

[Chris Beaven]

Can someone compare #3099 and this and figure out the better one?

[gkelly@…]

I ran #3099 with the FireBug profiler and did not see any increased performance compared to the existing implementation. This patch (#3202) uses a very well-known javascript optimization technique, offline DOM manipulation, to achieve a 75% performance increase in my testing. This patch is also much simpler, with roughly 3-4 lines changed/added.

[Chris Beaven]

Thanks Grant.

I think #3099 must be a different issue altogether, haven't investigated as it doesn't have a diff. Perhaps you could take a look at it and provide some comments if you have the time?

[Graham King <graham@…>]

I've closed #3099 (I originally opened it) as in Firefox it is quite a lot slower than this version.

In Internet Explorer 6 moving an item from one box (about 4500 items in it) to the other takes over 30 seconds (timed on my stopwatch), whereas it takes about 4 seconds in Firefox, and about 2 with this patch. Filtering is also very very slow. The JS interpreter in Firefox seems vastly superior.
I vote this patch goes in, and I'll try and see if I can improve the speed on IE and maybe submit a patch later.

[gkelly@…]

Replying to Graham King <graham@darkcoding.net>:

In Internet Explorer 6 moving an item from one box (about 4500 items in it) to the other takes over 30 seconds ...

I agree. IE doesn't seem to benefit from this patch at all. (Try it in plain Mozilla for a blazing fast experience!)

[Gary Wilson <gary.wilson@…>]

Just noting that while #3099 might be slower, it does sort correctly and also fixes #348. The patch attached to this ticket does neither.

The resulting patched files using the patches from each ticket are not that different. I think it would be nice to take the best of both and combine them into one. I think that would result in the speed of this patch with the correct sorting and add/remove from cache of #3099's patch, closing all three of this tickets (#3202, #3099, #348).

[James Bennett]

Reassigning to xian since he's working on newforms-admin JS.

[Yeago]

So it doesn't speed up IE: big deal! Lots of other browsers have gained momentum since this patch was written and its possible it will help some or all of them.

[Yeago]

Misunderstood patch limitations. Looking at it meanwhile to see if it still might apply.

[Yeago]

So I did some testing on a queryset of 14k. FF is *way* faster. IE and Chrome are unnoticably different.

I'm not sure what benefit the other tickets that were merged into this one may have had, but I don't really see anything stopping this patch from being applied. It doesn't seem to revert any features currently in trunk.

[Matthias Kestenholz]

Related ticket for the same issue (with seemingly more thorough, but not-being-worked-on patch): #9102 admin Filter Box redraws each time it's changed

[Julien Phalip]

#9102 was closed as a duplicate and has a patch with a different approach.

[illogical]

The line

        box.options.length = 0; // clear all options 

takes several seconds to complete with just about 2.5k entries in the select box on IE9, other IE versions seem to be affected as well.
Right now I don't have time for this, but maybe I can provide some test results later on.

It seems to perform way better when I replace this with

        while (box.hasChildNodes()) {
                box.removeChild(element.firstChild());
        }

which also seems much more sane to me.
Another way might be to set innerHTML to "".

[illogical]

Replying to illogical:

        while (box.hasChildNodes()) {
                box.removeChild(element.firstChild());
        }

This should of course be

         while (box.hasChildNodes()) {
                 box.removeChild(box.firstChild);
         }

sorry for the double post.

[Simon Charette]

innerHTML = '' should be way faster.

[myusuf3]

This patch doesn't apply cleanly.

[Illogical]

tried to optimize dom manipulation. needs testing.

[KyleMac]

I opened a pull request over at Github (​https://github.com/django/django/pull/222) that massively increases performance for me. It can transfer items from a list 11,500 long almost instantly.

[Tim Graham]

Since Julien asked the code to be rewritten with jQuery as part of this change, I'm closing this ticket as a duplicate of #15220.