Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.tabledrag.js has always been one of the items in Drupal which I believe needs to be optimized. It falls apart very quickly in large menu's and taxonomies, making those admin pages almost unusable for site administrators.
With that said, I just noticed a new patch on D6.26 which dealt with tableDrag and some for loops. Today I was searching around for various items related to Google Closure compiler and ran across this article:
Here's the snippet from the Google Closure: How not to write JavaScript article:
The Slow Loop
for (var i = fromIndex; i < arr.length; i++) {This for loop looks up the .length property of the array (arr) each time through the loop. Simply by setting a variable to store this number at the start of the loop, you can make the loop run much faster:
for (var i = fromIndex, ii = arr.length; i < ii; i++) {
Remembering the code I saw in the tableDrag.js patch I recalled it was doing this. I created a profiling script for tabledrag.js to see how many gratuitous .length calls are being done that could be cached.
Here's the recent D6.26 patch which was applied:
#800968: tabledrag.js should not use for...in to iterate over an array
I've performed this simple test, which involves editing the "Navigation Menu" on an existing site I have and dragging the "Logout" link on the bottom to the top of the menu. Be no means scientific, but something you can also easily test. Here are the results from this test:
Required calls to .length = 455
Actual calls to .length = 33478
This is pretty stunning, and while I'm not sure if it would help tabledrag.js, I think that file needs all the love it can get. This should be a simple fix and should not cause any problem if implemented properly. Will this fix all the problems with tabledrag.js? Probably not. Is it a simple step in the right redirect? I think so.
More reading on speeding up tabledrag.js
#1419968: Replace $('selector', domelement) with $(domelement).find('selector')
| Comment | File | Size | Author |
|---|---|---|---|
| #43 | c20170405_174136.png | 44.86 KB | droplet |
| #34 | tabledrag_js_reduce-1571814-34.patch | 53.57 KB | craigada |
| #34 | interdiff-1571814-31-34.txt | 7.19 KB | craigada |
| #32 | tabledrag_js_reduce-1571814-31.patch | 7.19 KB | craigada |
| #32 | interdiff-1571814-19-31.txt | 6.85 KB | craigada |
Comments
Comment #1
nod_Fix in 8 first please :)
That's a very welcome change. I'm not sure how much of a performance improvement we'll get but that'll be cleaner anyway.
Comment #2
nod_And if you put a patch like that be sure to put it for 7.x or 8.x I don't watch the 6.x js queue.
Comment #3
DamienMcKennaTagging, this is a simple change that could be backported to both D7 and D6.
Comment #4
droplet CreditAttribution: droplet commentedhow large is it ? and on which browsers.
add/remove DOM and change styles (add/remove class) have a big performance issue that's hard to optimized.
benchmarks
http://jsperf.com/caching-array-length
Comment #5
j0rd CreditAttribution: j0rd commented@droplet 100s of items, but I think we can use the standard navigation menu as an example of something that could be sped up. Currently on a site I have open, there's 150 items in there. Once you install some modules, this menu gets fairly large. The sites I'm talking about for which tabledrag is slow, have much larger menu's or taxonomies.
The link to the benchmark you sent, has a very small data set. Caching .length would obviously provide more benefit on larger data sets and I assume could be potentially harmful on smaller datasets. The expensive of .length will obviously depend on the browser.
Here's a benchmark with a larger dataset (~200):
http://jsperf.com/caching-array-length/86#bs-logo
Compare the second value (red) to the second to last value (green), you'll noticing caching is quicker in almost every browser.
I've created another patch, which adds logging the counts of the arrays each time, so we can see which (if any) should be optimized and I think I've found a silver bullet.
This line appears to get called the most and I believe contains all the rows in the menu. If we simply cached this value, I think we'd be in good shape. Also I think you might only need to cache it once per page request, but adding items via ajax (maybe via a contrib) could mess it up.
All the other .length points appear to have small .lengths and should probably be left alone (under 10).
Download attached patch, apply it and open up your console.log and report back with your results.
Comment #6
droplet CreditAttribution: droplet commented@j0rd,
I also think it worth to cached length. even small dataset, on SS & IE8 have improves.
But im interested why you're hit performance issues. I tired to generate 1000's terms and tested on FF/Chrome/IE9. Dragging terms are very smooth.
Comment #7
j0rd CreditAttribution: j0rd commentedHere's a patch for D8, which has arguably the fastest for loop from what I can tell.
Comment #8
nod_Can you profile before/after the patch? I'm not sure this is the main bottleneck of tabledrag, also can you replace
++nbyn += 1please? We're not using++xanywhere else and I don't want to introduce it.The other for loops will be changed at some point to use that as well.
(edit) actually if you're up for it, this is a global change, we need a patch changing all for loops :)
Comment #9
j0rd CreditAttribution: j0rd commented++n is the fastest increment in JS and allows your loop to do a couple additional increments in a time cycle.
http://jsperf.com/fastest-way-to-increment
I also recall my old C teacher tell me the same thing, but I'm personally not a fan of how it looks, so I don't use it much in my code.
Could you link me to the reasoning behind using += 1? Curious for my own learning purposes. Quick google search says js optimizers can optimize this with out confusion. Are we using a JS optimizer (google-closure / JSMin something) in D8?
Also I was thinking about a proper way to profile the tabledrag.js, since @_nod appears to be the authority on this, could you also enlighten me on the proper process with regards to Drupal.
Sorry to be a pain asking for links, but I need to bring myself up to speed with D8 before I can submit patches for it.
Comment #10
nod_So the benchmark you're linking to actually shows that it really doesn't matter how you increment it. Within their error margins they are all equals. So performance-wise there is no "better" solution. Actually, just transform a for-in loop into a proper for loop (when it's possible) and you'll make any increment choice negligible.
I don't quite like how it look either but when I see it I have no doubts whatsoever about what it does. ++n, n++ would return different values and I really don't want to think about which one it is, even if it's pretty simple. As a maintainer I got other things to worry about.
This is not a performance choice, it's a clarity choice. Once again Douglas Crockford has the best justification: http://stackoverflow.com/questions/971312/why-avoid-increment-and-decrem... and http://www.jslint.com/lint.html
The profiling process is a bit dirty :) You'll have to comment the Drupal.attachBehavior() in drupal.js, load the page, start the profiler, run Drupal.behaviors.tabledrag.attach(document, Drupal.settings);, stop profiler and see how much time was spent in this function, for the normal and for the patched version. Ideally it show a good improvement, and we'll have an idea about how much improvement we're talking about (anything you find, multiply that by 10 and you got the improvement on mobile).
About tabledrag my money is on making for loops instead of using .each() to have a significant loop-related performance boost.
Comment #11
j0rd CreditAttribution: j0rd commented@droplet #6. My testing is with regards to D6/D7 and not D8, so this could be caused by the version of jquery primarily. Any optimizations to tabledrag with out reducing code clarity would be a good idea though. I've ran into this issue on many sites in the past, and current sites I'm developing or maintaining. I've done some static site re-implementations in Drupal and have used book module in the past to implement hierarchy / sub-folders. Importing thousands of nested pages, book module because of tabledrag becomes un-usable and I had to scrap it. Re-implemented with plain old menu's, and had to break every root subfolder, into a different menu to have the site be manageable by the end admin.
@nod_ thanks for taking the time to reply.
From looking into it a little more i++ vs ++i in the past came down to the first being 4 assembly instructions and the later being 2. Depends on your compiler though, in this case our JS engine.
As for profiling this, I think you need to actually fake force some moving of the items, as this is where you'd want to notice a reduction in profile time. I don't think simply attaching it would help. I did attempt doing some fake profiling by starting the profiler and dragging some items around, then stopping and taking a look, but the results were negligible, with both cases being under 1ms for length calls if I was reading the profiler correctly (all kind of new to me)
The largest issue I have with tabledrag is when I grab a group of items (like a parent with some children) and try and drag those around a large list. I think this has to do with the math in mousedown type functions, although from looking at it, I can't see any really good places to optimize.
Another place I did notice which could probably be tweaked and by searching the issue queue has never been talked about is the scrollSettings.interval, which is currently set to 50ms or 20 calls a second. This could probably be tweaked to be reduced with out causing much of a noticeable difference, other than cycled used.
Going to look into it more and report back. I think what we really need is a good profiling suite + tests, so we can see if what we do is helping or hurting.
--
PS. Another thing to keep in mind, us developers probably have beefier computers and the most up to date browsers. This will not be the case for site admins though, who could have some old P3/P4 computers running IE. So any items which we might notice gains, could help others and should probably be implemented, if there's no down side.
Comment #12
nod_About this performance thing I think overall you're looking in the wrong place. Actually, that is why you don't see anything in your profiler, it's just too small.
I just profiled the drag thing, what should be optimized, in order: findDropTargetRow, checkScroll, dragRow everything else is negligible until those 3 functions are optimized. About findDropTargetRow, first line of the function:
var rows = $(this.table.tBodies[0].rows).not(':hidden');this does a full for-in over all rows, then we loop over all the elements another time next line. Do the hidden check (the actual hidden check is not free either) in the other loop and boom, you'll see an improvement.This .length patch is for maintainability and consistency more than anything.
In JS it's the DOM that is slow, This is what needs to be optimized first and foremost. for-in, for, .length are important, next.
Comment #13
sunAny programmer should understand what i++ and ++i are doing and what the difference is. Nothing wrong with C-style pre- and post-increment and decrement operators, and there's no reason to not use them.
Comment #14
nod_Well, just stick with
n++then please. We do use it in other loops, we're not using++nanywhere else in the JS.Comment #15
cweagansFixing tags per http://drupal.org/node/1517250
Comment #15.0
cweagansimproved link.
Comment #16
xjmComment #17
nod_Not exactly novice but a little challenge never hurts.
Comment #18
himanshupathak3 CreditAttribution: himanshupathak3 as a volunteer and at Srijan | A Material+ Company commentedComment #19
himanshupathak3 CreditAttribution: himanshupathak3 as a volunteer and at Srijan | A Material+ Company commentedComment #20
himanshupathak3 CreditAttribution: himanshupathak3 as a volunteer and at Srijan | A Material+ Company commentedComment #21
droplet CreditAttribution: droplet commentedNo significant improves for D8 ( targeted to modern browsers)
Comment #22
alexpottSo there are another 2 instances of .length in a for loop in the same file - I think we should we be changing all of them or not at all. But as @droplet has noted there is no significant improvements... is this worth doing. There's at least another 30 instances in core...
Comment #23
himanshupathak3 CreditAttribution: himanshupathak3 as a volunteer and at Srijan | A Material+ Company commented@alexpott so we should update in all instances of core files including this file also ?
Comment #24
alexpott@himanshupathak3 I'm not sure - I'd like input from @nod_ and @droplet as to whether they feel the effort is worth it.
Comment #25
nod_It is a minor task, and the profiling done in #12 didn't show this to have a significant perf improvment.
I say we should leave this one for 8.1.x, we have many non-minor bugs in the queue.
Comment #27
droplet CreditAttribution: droplet commentedI have the same comment as #21.
Anyway, last patch `len` should not a global varNeeds move it to single line of each var.Comment #28
alexpottThere is also some debate about whether the new formulation is actually faster... https://stackoverflow.com/questions/17989270/for-loop-performance-storin...
Comment #31
craigada CreditAttribution: craigada commentedComment #32
craigada CreditAttribution: craigada at La Drupalera by Emergya commentedApply the patch and change the rest of for loops.
Comment #34
craigada CreditAttribution: craigada at La Drupalera by Emergya commentedFix code standards errors.
Comment #36
droplet CreditAttribution: droplet commentedIt's 2017, everything is diff. Welcome to reopen with new thoughts :)
Comment #37
craigada CreditAttribution: craigada commentedComment #38
AnybodyThe tabledrag functionality is still performing bad with a lot of elements (> 150 is already too much). So a backport to D7 would be very helpful. If this does no more apply for D8 perhaps, at least it matched for D7 and really needs a backport.
Comment #39
AnybodySorry I have to correct myself. After testing this, the change doesn't cause the performance issues for me. So it may help but is truely minor. Sorry for replying too early.
Comment #40
droplet CreditAttribution: droplet commentedOK @Anybody.
Can you test patch #32 and tell us if it helps. If not, I think it's better to open a new issue with more NEW researching. It will save us to read 40 unrelated comments. :)
The thing I've seen, there's more like slowness (on IE) on the DOM rather than JS scripting. Something like virtual DOM may help.
and the problem on
If we commented out the line to swap the DOM, it's less than 6ms in IE11.
So we may add _.debounce to there for IE, that will help a bit but not perfect
Thoughts?
Comment #41
droplet CreditAttribution: droplet commentedJust read your comment after my posting. Can you tell us which browser & patch do you testing? I did a test on Chrome, there's no diff. IE is broken with patching
Comment #42
AnybodyI tested patch #32 on Firefox 52 and the results are the same... my performance issue seems to be based on the weight select field somehow with many rows... that's a new story. I think you're right, this issue can be closed.
Comment #43
droplet CreditAttribution: droplet commentedThe real problem is here:
Comment #44
droplet CreditAttribution: droplet commentedCome up a simple workaround: #2867451: Improve tabledrag.js performance