Opened 17 years ago

Closed 11 years ago

#3202 closed New feature (duplicate)

[patch] Faster SelectBox for ManyToMany

Reported by: gkelly@… Owned by: xian
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: Yeago Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (4)

SelectBox.js.patch (1.3 KB ) - added by anonymous 17 years ago.
Patch to SelectBox.js
original.png (89.8 KB ) - added by anonymous 17 years ago.
Javascript profiler for original SelectBox.js
faster.png (89.6 KB ) - added by anonymous 17 years ago.
Javascript profiler for faster SelectBox.js
SelectBox.patch (956 bytes ) - added by Illogical 12 years ago.
tried to optimize dom manipulation. needs testing.

Download all attachments as: .zip

Change History (26)

by anonymous, 17 years ago

Attachment: SelectBox.js.patch added

Patch to SelectBox.js

by anonymous, 17 years ago

Attachment: original.png added

Javascript profiler for original SelectBox.js

by anonymous, 17 years ago

Attachment: faster.png added

Javascript profiler for faster SelectBox.js

comment:1 by Gary Wilson <gary.wilson@…>, 17 years ago

see also #3099.

comment:2 by Chris Beaven, 17 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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

in reply to:  2 comment:3 by gkelly@…, 17 years ago

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.

comment:4 by Chris Beaven, 17 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

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?

comment:5 by Graham King <graham@…>, 17 years ago

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.

in reply to:  5 comment:6 by gkelly@…, 17 years ago

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!)

comment:7 by Gary Wilson <gary.wilson@…>, 17 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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).

comment:8 by James Bennett, 17 years ago

Owner: changed from nobody to xian

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

comment:9 by Yeago, 14 years ago

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.

comment:10 by Yeago, 14 years ago

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

comment:11 by Yeago, 14 years ago

Patch needs improvement: unset

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.

comment:12 by Yeago, 14 years ago

Cc: Yeago added

comment:13 by Matthias Kestenholz, 13 years ago

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

comment:14 by Łukasz Rekucki, 13 years ago

Severity: normalNormal
Type: enhancementNew feature

comment:15 by Julien Phalip, 13 years ago

Easy pickings: unset
UI/UX: unset

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

comment:16 by illogical, 12 years ago

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 "".

in reply to:  16 comment:17 by illogical, 12 years ago

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.

comment:18 by Simon Charette, 12 years ago

innerHTML = '' should be way faster.

comment:19 by myusuf3, 12 years ago

Patch needs improvement: set

This patch doesn't apply cleanly.

by Illogical, 12 years ago

Attachment: SelectBox.patch added

tried to optimize dom manipulation. needs testing.

comment:20 by KyleMac, 12 years ago

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.

comment:21 by Tim Graham, 11 years ago

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.

comment:22 by Tim Graham, 11 years ago

Resolution: duplicate
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top