Skip to content

Conversation

@hdgarrood
Copy link
Contributor

Like #48, but rebased, and I fixed a minor error, and added some more tests that would have caught it. /cc @S11001001

case 1: return map(array1)(f(array[bot]));
case 2: return apply(map(array2)(f(array[bot])))(f(array[bot + 1]));
default:
var pivot = Math.floor((top + bot) / 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for the trickier pivot selection was because e.g. for 6 elements, a pivot of 2 is superior to a pivot of 3. Can we preserve this?

cc @syaiful6

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, thanks. So would it make sense to add a branch to our switch statement for when top - bot == 6? Can you explain the general principle behind the trickier pivot selection? I can't quite see how to modify your code to get it to work other than perhaps to leave it alone and add a branch to the switch statement for when top - bot == 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, please feel free to take these commits and put them back into your PR if you want, and then I could close this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or at least the ones where I added more tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is simply to choose pivots that will yield two even-length partitions where possible.

I suppose the best approach would be to add a case 3. IIUC this is the only nonterminating case under #48; 4 will be divided into 2 and 2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hdgarrood OK, I'll pull your changes back. Thanks for all the fixups.

@hdgarrood hdgarrood closed this Nov 17, 2016
@hdgarrood hdgarrood deleted the divide-and-conquer-traverse branch December 24, 2016 17:53
@hdgarrood hdgarrood reopened this Jan 3, 2017
Also add comment to explain why it is this way.
@hdgarrood hdgarrood force-pushed the divide-and-conquer-traverse branch from 0391706 to a251fde Compare January 3, 2017 15:35
@hdgarrood hdgarrood force-pushed the divide-and-conquer-traverse branch from 8fc95fa to e7cb46c Compare January 3, 2017 15:39
@hdgarrood
Copy link
Contributor Author

hdgarrood commented Jan 3, 2017

I've made the changes to pivot selection discussed earlier in this PR, and double checked that this is not a breaking change (note that we only changed devDependencies in bower.json). Also, now that integers exports an int pow function, I've updated the tests to use that instead of defining it here using the FFI. I think this is good to merge now.

@garyb
Copy link
Member

garyb commented Jan 3, 2017

Did we benchmark it at all in the end?

@hdgarrood
Copy link
Contributor Author

We did, I added a comment in #48 with results. Performance wise it seems to be pretty much the same as the previous implementation, at least on V8.

@S11001001
Copy link
Contributor

@hdgarrood I'm glad it worked; it didn't in my own tests.

The asymptotics have several dimensions, each induced by particular applicatives. Some examples:

  1. A Writer with O(n) append will multiply out to O(n²) with the original implementation, but be O(n log₂n) with divide-and-conquer.
  2. A Writer whose monoid is fast when folding one way but slow when folding the other way will be somewhere in between. For example, a cons list monoid with the elements more-or-less distributed evenly will be O(n) with a right-folding traversal, O(n²) with a left-folding traversal, and O(n log₂n) with divide-and-conquer. A monoid that's slow when right-folding, but fast when left-folding, also ends up with divide-and-conquer as a middle ground.
  3. For Reader there's no difference when traversing, and no performance difference when invoking the resulting function. But invoking the resulting function will consume O(n) stack for the original implementation (even if "stack-safe", hence the error added by the test), but O(log₂n) stack for divide-and-conquer. Note that to observe this, you have to invoke the function; the performance of calling traverse won't include this information.

@garyb
Copy link
Member

garyb commented May 22, 2018

Resolved by #78 (!)

@garyb garyb closed this May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants