Code

Opened 8 years ago

Closed 12 months 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 8 years ago.
Patch to SelectBox.js
original.png (89.8 KB) - added by anonymous 8 years ago.
Javascript profiler for original SelectBox.js
faster.png (89.6 KB) - added by anonymous 8 years ago.
Javascript profiler for faster SelectBox.js
SelectBox.patch (956 bytes) - added by Illogical 2 years ago.
tried to optimize dom manipulation. needs testing.

Download all attachments as: .zip

Change History (26)

Changed 8 years ago by anonymous

Patch to SelectBox.js

Changed 8 years ago by anonymous

Javascript profiler for original SelectBox.js

Changed 8 years ago by anonymous

Javascript profiler for faster SelectBox.js

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

see also #3099.

comment:2 follow-up: Changed 7 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 7 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 7 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 7 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 7 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 7 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 7 years ago by ubernostrum

  • Owner changed from nobody to xian

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

comment:9 Changed 4 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 4 years ago by subsume

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

comment:11 Changed 4 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 4 years ago by subsume

  • Cc subsume added

comment:13 Changed 4 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 3 years ago by lrekucki

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

comment:15 Changed 3 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 2 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 2 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 2 years ago by charettes

innerHTML = '' should be way faster.

comment:19 Changed 2 years ago by myusuf3

  • Patch needs improvement set

This patch doesn't apply cleanly.

Changed 2 years ago by Illogical

tried to optimize dom manipulation. needs testing.

comment:20 Changed 2 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 12 months 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 12 months ago by timo

  • Resolution set to duplicate
  • Status changed from new to closed

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.