Opened 10 years ago

Closed 3 years ago

#3202 closed New feature (duplicate)

[patch] Faster SelectBox for ManyToMany

Reported by: gkelly@… Owned by: xian
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: subsume 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 10 years ago.
Patch to SelectBox.js
original.png (89.8 KB) - added by anonymous 10 years ago.
Javascript profiler for original SelectBox.js
faster.png (89.6 KB) - added by anonymous 10 years ago.
Javascript profiler for faster SelectBox.js
SelectBox.patch (956 bytes) - added by Illogical 4 years ago.
tried to optimize dom manipulation. needs testing.

Download all attachments as: .zip

Change History (26)

Changed 10 years ago by anonymous

Patch to SelectBox.js

Changed 10 years ago by anonymous

Javascript profiler for original SelectBox.js

Changed 10 years ago by anonymous

Javascript profiler for faster SelectBox.js

comment:1 Changed 10 years ago by Gary Wilson <gary.wilson@…>

see also #3099.

comment:2 follow-up: Changed 10 years ago by SmileyChris

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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

comment:3 in reply to: ↑ 2 Changed 10 years ago by 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.

comment:4 Changed 10 years ago by SmileyChris

  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready 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 follow-up: Changed 10 years ago by 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.

comment:6 in reply to: ↑ 5 Changed 10 years ago by 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!)

comment:7 Changed 10 years ago by Gary Wilson <gary.wilson@…>

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 Changed 9 years ago by ubernostrum

  • Owner changed from nobody to xian

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

comment:9 Changed 6 years ago by subsume

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 Changed 6 years ago by subsume

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

comment:11 Changed 6 years ago by subsume

  • 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 Changed 6 years ago by subsume

  • Cc subsume added

comment:13 Changed 6 years ago by mk

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 Changed 5 years ago by lrekucki

  • Severity changed from normal to Normal
  • Type changed from enhancement to New feature

comment:15 Changed 5 years ago by julien

  • Easy pickings unset
  • UI/UX unset

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

comment:16 follow-up: Changed 4 years ago by 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 "".

comment:17 in reply to: ↑ 16 Changed 4 years ago by 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.

comment:18 Changed 4 years ago by charettes

innerHTML = '' should be way faster.

comment:19 Changed 4 years ago by myusuf3

  • Patch needs improvement set

This patch doesn't apply cleanly.

Changed 4 years ago by Illogical

tried to optimize dom manipulation. needs testing.

comment:20 Changed 4 years ago by 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.

comment:21 Changed 3 years ago by timo

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 Changed 3 years ago by timo

  • Resolution set to duplicate
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top