Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite reorder (part 2) #199

Merged
merged 4 commits into from Mar 5, 2015

Conversation

@neonstalwart
Copy link
Collaborator

commented Feb 27, 2015

@Matt-Esch this is some extra work on top of what you've done in #197. please take the time to convince yourself that this is reasonable - i don't want to get this wrong again. i have relied on tests to prove this code as being correct but that's only as good as the tests themselves.

things of note:

  • added some fuzzy testing - please check what i've done with the tests and make sure it's reasonable because i'm depending on these tests to make sure things are working. there are some specific cases also included to exercise some corner cases and to make debugging/development easier.
  • allowed for left-to-right moves when reordering. this means that a change that moves one key from position 4 to position 7 is now just [{ from: 4, to: 7}] rather than [{ from: 5, to: 4 }, { from: 6, to: 5 }, { from: 7, to: 6 }]
  • removed while(!sorted) loop which i think was previously making a reorder O(N3) (O(N) for the while loop * O(N) for the for loop * O(N) for searching for the node to move) and now it's O(N2) (O(N) for the for loop * O(N) for searching for the node to move)

this PR supersedes or fixes #191, #194, #197, #198

as i mentioned in #197, i think this change warrants a major version bump since the output format of a diff is changing in a backwards incompatible way (but i think it's definitely the right thing to do). in doing that, let's look at what else may be something to address in a major version bump (e.g. stop allowing whitespace in selectors as discussed in #180)

@coveralls

This comment has been minimized.

Copy link

commented Feb 27, 2015

Coverage Status

Coverage increased (+0.02%) to 97.29% when pulling 700e4fd on neonstalwart:rewrite-reorder into 407ec42 on Matt-Esch:rewrite-reorder.

@@ -343,16 +348,23 @@ function reorder(aChildren, bChildren) {
// TODO: We really want to make this more efficient
// Finding the current index of any key in the array could be
// faster.
function moveItem(array, toIndex, itemKey) {
function indexOfKey(array, itemKey) {

This comment has been minimized.

Copy link
@Zolmeister

Zolmeister Feb 27, 2015

Contributor

Can't we use a hash for this? Reducing the complexity to O(N) would be great.

Don't do this, but a hack idea:

function indexOfKey(array, itemKey) {
  if (array.keyMap) return array.keyMap[itemKey]
  for (i in array) {
    array.keyMap[array[i].key] = i
  }
return array.keyMap[itemKey]

And it's updated on array mutation before first method call

This comment has been minimized.

Copy link
@neonstalwart

neonstalwart Feb 27, 2015

Author Collaborator

the updating after a mutation is O(N) which is the same as not keeping the hash and scanning the list like we do now - the hash is technically worse (by just a little bit) since it requires more memory.

This comment has been minimized.

Copy link
@neonstalwart

neonstalwart Feb 27, 2015

Author Collaborator

i.e. scanning the list is already O(N)

This comment has been minimized.

Copy link
@Zolmeister

Zolmeister Feb 27, 2015

Contributor

Updated: just run once before a patch cycle

This comment has been minimized.

Copy link
@Zolmeister

Zolmeister Feb 27, 2015

Contributor

Well, that, and keeping it updated incrementally

This comment has been minimized.

Copy link
@neonstalwart

neonstalwart Feb 27, 2015

Author Collaborator

the list mutates during the patch so it has to update after each mutation... which is no more efficient than looking for the item each time we mutate (like we do now).

...and to cut off another thought, array.indexOf is likely to perform just the same as what we're doing but only some performance tests can really prove if it's faster.

This comment has been minimized.

Copy link
@neonstalwart

neonstalwart Feb 27, 2015

Author Collaborator

keeping it updated incrementally

the incremental updates are O(N)... still the same. i've given this a lot of thought this week 😄

This comment has been minimized.

Copy link
@Zolmeister

Zolmeister Feb 27, 2015

Contributor

I'm sure this is possible. React does it in O(N) with keys: http://facebook.github.io/react/docs/reconciliation.html
I'll think about it more.

This comment has been minimized.

Copy link
@neonstalwart

neonstalwart Feb 27, 2015

Author Collaborator

i just took a look at what react does and we could achieve O(N) if we were to sacrifice the same things they have given up.

take this example:

React.render(
    <ul>
      <li key="1" id="1">1</li>
      <li key="2" id="2">2</li>
      <li key="3" id="3">3</li>
      <li key="0" id="0">0</li>
      <li key="4" id="4">4</li>
  </ul>,
  document.getElementById('example')
);

setTimeout(function () {
  React.render(
      <ul>
          <li key="0" id="0">0</li>
          <li key="1" id="1">1</li>
          <li key="2" id="2">2</li>
          <li key="3" id="3">3</li>
          <li key="4" id="4">4</li>
      </ul>,
      document.getElementById('example')
  );
}, 500)

intuitively there should be one move to get the element with key="0" into it's final position (from 3, to 0). however, react instead produces 3 moves (from 0, to 1; from 1, to 2; from 2, to 3). moving all these nodes around has some cost associated with it and will trigger blur handlers and break animations on those nodes. the question is then whether the cost of calculating the efficient moves outweighs the cost (and breaking developer's expectations of nodes remaining in the DOM) of moving lots of nodes in the DOM. intuitively i believe that for a typical number of keyed nodes (a low number), calculating the efficient patch and keeping more nodes in the DOM more often is the better option but we don't have any conclusive performance metrics to back this up. http://vdom-benchmark.github.io/vdom-benchmark/ seems to suggest that so far virtual-dom has produced generally significantly better performance than react.

i'd really like to be able to get O(N) working AND produce the same set of moves this code currently produces - i know it's possible but for now i'm favoring working code over the most performant code. when we can get the performant code to be robust then i think we should switch to it.

// item at current location moves left to right later
if (simulateItem && simulateItem.key && bKeys[simulateItem.key] > k) {
// only do the current move if it is left to right
if (indexOfKey(simulate, wantedItem.key) < k) {

This comment has been minimized.

Copy link
@Zolmeister

Zolmeister Feb 27, 2015

Contributor

index could be cached here, and passed into moveItem to avoid another lookup

This comment has been minimized.

Copy link
@neonstalwart

neonstalwart Feb 27, 2015

Author Collaborator

done. thanks.

@Matt-Esch

This comment has been minimized.

Copy link
Owner

commented Feb 28, 2015

@Zolmeister of course we could make this O(N) if we were to relax the conditions for the moves. We used to have an O(N) algorithm but it did not minimize swaps at all.

We could create a mapping of desired index to dom node in patch an O(N) operation requiring O(N) ish memory depending on how the hash is defined, and then scan left to right. If a node is not in place, use the hash to insertBefore that node.

That's a single left to right iteration with N comparisons, forcing each item into the correct place. As we already noted though, there is an unfortunate amount of pain to suffer when an item at the start needs to be at the end, because it gets shuffled with N-1 swaps to the end, when we could reduce it to just N swaps. The O(N^2) algorithm with 1 swap for small enough N is likely (but hasn't been proven to be) faster overall than the O(N) algorithm with N-1 swaps. We are assuming that rearranging the DOM nodes is an expensive operation, and the JS reordering algorithm operates over cheap to mutate data structures.

@Zolmeister

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2015

@Matt-Esch Thanks for explaining, definitely makes more sense now.

@Matt-Esch

This comment has been minimized.

Copy link
Owner

commented Feb 28, 2015

@neonstalwart I am going to take a good amount of time to read your diff and tests in particular. In the mean time I was wondering how you felt about this idea:

The previous steps before the simulation allow us to create an array of integers from 1 .. N. The array will be in the out-of-order version. So in the case that the first item gets moved to the end you might have an array of

[5, 0, 1, 2, 3, 4]

Note that sorting an array of integers is going to be more efficient than sorting an array of complex objects, and for sufficiently small N we can efficiently move items by shuffling the items manually instead of using splice...

Anyway, the algorithm I was thinking of would look at each item left to right. When it sees an item out of place it determines 2 things, a) what is the current index of the item that should be here. b) what is the index that this current item should be at.

a) is computed simply by finding the item
b) b is computed by counting enough items less than the current item.

We can only do b because we have a strict sequence from 0 ... N. That is if Item at index 3 is 7, I know that I must place 7 after 3, 4, 5, and 6.

We then either move the item that should be here to this index, OR we move the item that is currently here to the index it should be at. We make the largest move.

Here are the moves from my example algorithm:

sorting        [5,0,1,2,3,4]
move 0 to 5 => [0,1,2,3,4,5]
done


sorting        [1,2,3,4,5,0]
move 5 to 0 => [0,1,2,3,4,5]
done


sorting        [5,4,3,2,1,0]
move 5 to 0 => [0,5,4,3,2,1]
move 5 to 1 => [0,1,5,4,3,2]
move 5 to 2 => [0,1,2,5,4,3]
move 5 to 3 => [0,1,2,3,5,4]
move 5 to 4 => [0,1,2,3,4,5]
done


sorting        [0,1,2,3,4,5]
done


sorting        [3,4,5,0,1,2]
move 0 to 5 => [4,5,0,1,2,3]
move 0 to 5 => [5,0,1,2,3,4]
move 0 to 5 => [0,1,2,3,4,5]
done

And the annotated source + log statements:

// arr must be an (unordered) set of integers 0, 1, 2, ... N without gaps
function sort(arr) {
    var arrLength = arr.length;
    var moves = [];
    console.log('sorting       ', JSON.stringify(arr));

    for (var i = 0; i < arrLength; i++) {
        var item = arr[i];

        if (item !== i) {
            // This is the index of the item that should be at i
            var desiredItemIndex = 0;

            // This is the index that the current item should be at
            var swapDestination = 0;

            // So if i is currently 3, say, and the item at 3 is 7, we want to
            // see 4, 5 and 6 first. So we want to see (item - i - 1) items
            // which are less than 7 to the right of the current item.
            var lessThanSearchHalt = item - i - 1;
            var lessThanSearchCount = 0;


            for (var j = i + 1; j < arrLength; j++) {
                var compareItem = arr[j];

                if (compareItem < item) {
                    lessThanSearchCount += 1;

                    if (lessThanSearchCount === lessThanSearchHalt) {
                        swapDestination = j + 1;
                    }
                }

                if (compareItem === i) {
                    desiredItemIndex = j;
                }

                if (desiredItemIndex > 0 && swapDestination > 0) {
                    break;
                }
            }

            // Make the largest move
            if (swapDestination > desiredItemIndex) {
                moves.push(move(arr, i, swapDestination));
                // We still need to put the correct item at i.
                i--;
            } else {
                moves.push(move(arr, desiredItemIndex, i));
            }
        }
    }

    console.log('done\n\n')
    return moves;
}

// TODO: shuffle items for small enough N based on benchmarks.
function move(arr, from, to) {
    arr.splice(to, 0, arr.splice(from, 1)[0]);
    console.log("move", from, "to", to, '=>', JSON.stringify(arr));
    return {
        from: from,
        to: to
    };
}

sort([5, 0, 1, 2, 3, 4]);
sort([1, 2, 3, 4, 5, 0]);
sort([5, 4, 3, 2, 1, 0]);
sort([0, 1, 2, 3, 4, 5]);
sort([3, 4, 5, 0, 1, 2]);
@Matt-Esch

This comment has been minimized.

Copy link
Owner

commented Feb 28, 2015

Also another thing to bear in mind is that for short arrays we can actually cache the moves in a tree based on the numeric ordering!

@Zolmeister

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2015

Based on this solution here is a method to calculate the optimal number of moves required. Hopefully this can make it into the tests:

function minMoves(arr) {
  var longestSeq = 0

  for (var i = 0; i < arr.length; i++) {
    var cnt = 1
    var val = arr[i]
    for (var j = i + 1; j < arr.length; j++) {
      if (val + 1 === arr[j]) {
        val += 1
        cnt += 1
      }
    }
    longestSeq = Math.max(longestSeq, cnt)
  }

  return arr.length - longestSeq
}

// [1, 7, 6, 2, 5, 4, 3] - 4
// [1, 6, 4, 3, 5, 2, 7] - 5
@coveralls

This comment has been minimized.

Copy link

commented Feb 28, 2015

Coverage Status

Coverage increased (+0.04%) to 97.31% when pulling 401edc1 on neonstalwart:rewrite-reorder into 407ec42 on Matt-Esch:rewrite-reorder.

@neonstalwart

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 1, 2015

@Matt-Esch i need to rebase this against your updated rewrite-reorder branch (probably tomorrow, i'm mostly out of time today) but take a look at what i've done with reordering now... it's O(N)!!!

as you've mentioned elsewhere, it overlaps with insertion and deletion patches but we can address that separately

@neonstalwart neonstalwart force-pushed the neonstalwart:rewrite-reorder branch 2 times, most recently from ee84617 to bb7de5e Mar 2, 2015

@coveralls

This comment has been minimized.

Copy link

commented Mar 2, 2015

Coverage Status

Coverage increased (+0.02%) to 97.5% when pulling bb7de5e on neonstalwart:rewrite-reorder into 707bb56 on Matt-Esch:rewrite-reorder.

@neonstalwart

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 2, 2015

@Matt-Esch this has been rebased (and worked around a weird childNodes[childNodes.length] bug in webkit). i tried making the simulate array just an array of keys instead of objects to see if the performance improved but i didn't see anything significant.

on the topic of performance, here's what i've been doing:

NOTE: you only get to see the most recent output for any test with the same label. change https://github.com/vdom-benchmark/vdom-benchmark-virtual-dom/blob/19b55f15d409af152d03bb2aecee58235e423fc6/web/js/main.js#L11 any time you want to do a side-by-side comparison with a different test run. e.g. i had values like 'origin/master', 'origin/rewrite-reorder', 'neonstalwart/rewrite-reorder', 'local', etc. so that i could run the benchmark with different code and compare them side-by-side. as an aside, i wish you could manipulate the name with the queryString so that you could simply put in a URL like http://localhost:3000/?name=origin/rewrite-reorder

@coveralls

This comment has been minimized.

Copy link

commented Mar 2, 2015

Coverage Status

Coverage increased (+0.01%) to 97.49% when pulling 92e3a3d on neonstalwart:rewrite-reorder into 707bb56 on Matt-Esch:rewrite-reorder.

@neonstalwart neonstalwart referenced this pull request Mar 3, 2015
@Matt-Esch

This comment has been minimized.

Copy link
Owner

commented Mar 3, 2015

Didn't get a chance to look at this, review coming up.

for (var j = 0; j < moves.inserts.length; j++) {
insert = moves.inserts[j]
node = keyMap[insert.key]
// this is the weirdest bug i've ever seen in webkit

This comment has been minimized.

Copy link
@Matt-Esch

Matt-Esch Mar 3, 2015

Owner

it's probably undefined behavior. Maybe there is a better way than checking the child node length? Don't really want to be checking a host object property in a tight loop when we knew upfront that it was an append operation.

This comment has been minimized.

Copy link
@neonstalwart

neonstalwart Mar 3, 2015

Author Collaborator

i'll improve this to avoid referencing childNodes.length - i get the reference once before the loop and then maintain my own counter

@Matt-Esch

This comment has been minimized.

Copy link
Owner

commented Mar 3, 2015

One thing I've noticed is that this algorithm removes items that are visible. At first I thought this was terrible but I'm not sure it really is. I would like to know if this has any effect on transitions, focus or other visible rendering in any browser.

Essentially the only difference between the algorithm I proposed and this one is that you're using a remove operation as a stack instead of eagerly inserting the node into the correct position. This is probably going to increase the number of DOM operations by some amount, and I want a proper test with a chrome profile before choosing.

FWIW looking at an array of numbers is way easier to debug, but I'm not overly committed to it, I care more about reducing DOM operations and overall render time.

I think we should test shuffle performance of a todomvc list and look at a chrome profile.

@neonstalwart

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 3, 2015

One thing I've noticed is that this algorithm removes items that are visible. At first I thought this was terrible but I'm not sure it really is.

do you have any examples of where something is happening that should be avoided? it should be simple enough to add a test to ensure we have what is desirable.

there will always be some ambiguous cases where an existing node has to move and we need to arbitrarily choose one node to move instead of another.

for example, if we start with [0,1,2,4,3,5] and we see a change to [0,1,2,3,4,5], a user could have intended either of:

  • move 4 after 3 - the expectation might be that 4 would be the element leaving the DOM
  • move 3 before 4 - the expectation might be that 3 would be the element leaving the DOM

we don't have enough information to know which one is intended and so we leave it to the developer of the code to manage transitions etc to match the intent of the user and we just make sure that the final state of the DOM is consistent with what we know.

@coveralls

This comment has been minimized.

Copy link

commented Mar 3, 2015

Coverage Status

Coverage increased (+0.02%) to 97.49% when pulling f633080 on neonstalwart:rewrite-reorder into 707bb56 on Matt-Esch:rewrite-reorder.

@neonstalwart

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 3, 2015

Essentially the only difference between the algorithm I proposed and this one is that you're using a remove operation as a stack instead of eagerly inserting the node into the correct position.

by using the stack i can do the shuffle in O(N) (worst case is N+M, i.e. bChildren.length + simulate.length but that is still called O(N)) compared to O(N2) worst case, or O(NlogN) average case, for yours.

the other advantage is that my patch format lends itself to folding all removes and inserts into VPatch.ORDER to remove the redundancy with VPatch.INSERT and VPatch.REMOVE but this is not as big a deal as being able to reduce the big-O complexity.

@neonstalwart

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 3, 2015

FWIW looking at an array of numbers is way easier to debug,

i worked on a variation where simulate was an array of keys rather than an array of vnodes. it didn't seem to change the performance so i didn't do it but if you think it helps debugging, it's fairly trivial to switch to that.

@Matt-Esch

This comment has been minimized.

Copy link
Owner

commented Mar 4, 2015

It's also worth noting that objects are not really hashes in javascript and that v8 does a linear lookup on the object keys to find out where in memory it's located. I don't consider using a hash as O(1) in most cases and in the v8 case it's actually an O(N) lookup.

@Zolmeister

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2015

@Matt-Esch Can you provide a link to that info?

@Matt-Esch

This comment has been minimized.

Copy link
Owner

commented Mar 4, 2015

http://thlorenz.github.io/talks/demystifying-v8/talk.pdf

So the linear lookup is worst case. v8 "hashes" are objects in dictionary mode. This means that when you write a property, the string is hashed and first 2 bytes used to compute the index. From then on, we linearly search for the result looping back round like any other hash table implementation. This has worst case complexity of O(N).

They call objects in dictionary mode "slow mode" for a reason. This is probably too low level and v8 specific, but I do get frustrated when people think hash tables are O(1). It's only an O(1) lookup when you can go from string to offset without searching.

@Zolmeister

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2015

My bad, I forgot it requires a perfect-hash function to get O(1). (even the wikipedia page says O(n))
That being said, average case is still O(1), so we can both be right. (well, partially)

@neonstalwart

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 4, 2015

i think we all want the "best" implementation and i don't mind if that's not mine but it looks like we need to do some profiling. ideally we want tests that include patching the DOM but i'm stretched for time right now so maybe i can start by adjusting test/sort.js to do a diff using vtree/diff rather than just copying/pasting sort algorithms in that file. then we can at least profile both approaches in a way that is meaningful for comparison - although without patching the DOM it is not completely conclusive. after that maybe it won't take a lot of effort to add patching the DOM to that.

apart from performance, i think there were some characteristics of your approach which were undesirable. e.g. reordering nodes that will be removed but given the number of changes we've gone through in the past couple of weeks, i can't remember whether this is still the case or not so forgive me if i'm wrong.

@neonstalwart neonstalwart force-pushed the neonstalwart:rewrite-reorder branch from f633080 to 3ab690a Mar 4, 2015

@neonstalwart neonstalwart force-pushed the neonstalwart:rewrite-reorder branch from 3ab690a to 9c09d58 Mar 4, 2015

@neonstalwart

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 4, 2015

using the changes to test/sort.js added in d121124 from this branch and the below config. i got the log output from chrome included in the table below. (all the permutations were sorted correctly but i've removed the logging output related to that).

@Matt-Esch these numbers below show no significant performance difference between the 2 approaches. it would be good to double-check that the changes to test/sort.js are reasonable and verify my results by running the tests again but i'm happy to bring this to a conclusion any way you want to - i.e. i don't care too much which code you merge as long as we aren't compromising on our other constraints such as disturbing the DOM as little as possible.

var PERMUTATION_START = 1;
var PERMUTATION_END = 8;

var SAMPLE_START = 10;
var SAMPLE_END = 1000;
var SAMPLE_COUNT = 100;
var SAMPLE_INTERVAL = 100;
d121124 9c09d58
All (1) arrays sorted in 1 ms All (1) arrays sorted in 0 ms
An array of length 1 sorts in 1 ms An array of length 1 sorts in 0 ms
All (2) arrays sorted in 3 ms All (2) arrays sorted in 2 ms
An array of length 2 sorts in 1 ms An array of length 2 sorts in 1 ms
All (6) arrays sorted in 7 ms All (6) arrays sorted in 9 ms
An array of length 3 sorts in 1 ms An array of length 3 sorts in 1 ms
All (24) arrays sorted in 38 ms All (24) arrays sorted in 34 ms
An array of length 4 sorts in 1 ms An array of length 4 sorts in 1 ms
All (120) arrays sorted in 115 ms All (120) arrays sorted in 154 ms
An array of length 5 sorts in 0 ms An array of length 5 sorts in 1 ms
All (720) arrays sorted in 749 ms All (720) arrays sorted in 768 ms
An array of length 6 sorts in 1 ms An array of length 6 sorts in 1 ms
All (5040) arrays sorted in 5824 ms All (5040) arrays sorted in 6111 ms
An array of length 7 sorts in 1 ms An array of length 7 sorts in 1 ms
All (40320) arrays sorted in 55557 ms All (40320) arrays sorted in 52125 ms
An array of length 8 sorts in 1 ms An array of length 8 sorts in 1 ms
All (100) arrays sorted in 143 ms All (100) arrays sorted in 125 ms
An array of length 10 sorts in 1 ms An array of length 10 sorts in 1 ms
All (100) arrays sorted in 1120 ms All (100) arrays sorted in 1201 ms
An array of length 110 sorts in 11 ms An array of length 110 sorts in 12 ms
All (100) arrays sorted in 2204 ms All (100) arrays sorted in 2166 ms
An array of length 210 sorts in 22 ms An array of length 210 sorts in 21 ms
All (100) arrays sorted in 3055 ms All (100) arrays sorted in 3078 ms
An array of length 310 sorts in 30 ms An array of length 310 sorts in 30 ms
All (100) arrays sorted in 4079 ms All (100) arrays sorted in 4110 ms
An array of length 410 sorts in 40 ms An array of length 410 sorts in 41 ms
All (100) arrays sorted in 5013 ms All (100) arrays sorted in 5058 ms
An array of length 510 sorts in 50 ms An array of length 510 sorts in 50 ms
All (100) arrays sorted in 6011 ms All (100) arrays sorted in 6066 ms
An array of length 610 sorts in 60 ms An array of length 610 sorts in 60 ms
All (100) arrays sorted in 6981 ms All (100) arrays sorted in 7034 ms
An array of length 710 sorts in 69 ms An array of length 710 sorts in 70 ms
All (100) arrays sorted in 7906 ms All (100) arrays sorted in 8073 ms
An array of length 810 sorts in 79 ms An array of length 810 sorts in 80 ms
All (100) arrays sorted in 9069 ms All (100) arrays sorted in 9537 ms
An array of length 910 sorts in 90 ms An array of length 910 sorts in 95 ms
@Zolmeister

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2015

Can you output the number of moves each?

@neonstalwart

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 4, 2015

Can you output the number of moves each?

we have other tests for ensuring the number of moves are minimal. admittedly, those tests are not comprehensive but i'm open to a PR that improves on what we have. i've probably spent more than 60 hours on this over the past 2 weeks - my resources have a limit.

@Matt-Esch

This comment has been minimized.

Copy link
Owner

commented Mar 4, 2015

@neonstalwart all of the hard work appreciated as always. I have a test in the works that I am going to run through some profiling which will give us some indication of any performance overheads.

My main concern is the number of DOM operations performed. I was assuming that the algorithms have a similar performance profile but that the O(N) algorithm you suggested increases the number of DOM operations (an increase in removes though I don't expect more moves).

I am concerned that this will have a performance penalty in some browser we care about which outweighs any advantage of the O(N) algorithm. I want to stop using conjecture and actually profile this in the browser and land something. Afterall, sorting is completely broken on master right now, which itself outweighs the need to squeeze every last drop.

So, I will post chrome profiles and I will time how long the JS blocks for. I'll create a test and post the results for an array of browsers, and we will just pick the one that provides the best overall performance i.e. the one that gives us the most acceptable sort times across browsers.

@neonstalwart

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 5, 2015

FYI my profiling includes the DOM interactions.

Matt-Esch added a commit that referenced this pull request Mar 5, 2015

@Matt-Esch Matt-Esch merged commit 7009269 into Matt-Esch:rewrite-reorder Mar 5, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Matt-Esch

This comment has been minimized.

Copy link
Owner

commented Mar 5, 2015

yolo

@neonstalwart neonstalwart deleted the neonstalwart:rewrite-reorder branch Mar 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.