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

Fix bad edge case in bubble sort #23

Merged
merged 1 commit into from Nov 8, 2017

Conversation

Projects
None yet
3 participants
@islemaster
Member

islemaster commented Nov 8, 2017

Turns out the Array.prototype.sort polyfill, implemented as a bubble sort, is broken.

Brought to our attention by a Zendesk ticket and easily reproduced in App Lab with the following code:

// Array.sort doesn't work properly
console.log([5, 4, 5, 1].sort());
// Expected: [1,4,5,5]
// Actual  : [4,1,5,5]

The problem is that this Array.prototype.sort polyfill, which uses a bubble sort, is implemented incorrectly:

JS-Interpreter/interpreter.js

Lines 1100 to 1117 in 37933e4

"Object.defineProperty(Array.prototype, 'sort', {configurable: true, value:",
"function(opt_comp) {",
"for (var i = 0; i < this.length; i++) {",
"var changes = 0;",
"for (var j = 0; j < this.length - i - 1; j++) {",
"if (opt_comp ?" +
"opt_comp(this[j], this[j + 1]) > 0 : this[j] > this[j + 1]) {",
"var swap = this[j];",
"this[j] = this[j + 1];",
"this[j + 1] = swap;",
"changes++;",
"}",
"}",
"if (changes <= 1) break;",
"}",
"return this;",
"}",
"});",

The exact issue is the if (changes <= 1) break; line. Stepping through the algorithm for the broken case, we can see that we're breaking too early, when only one swap has occurred in the pass but there's still more work to do.

i is 0
  begin 5,4,5,1
  j is 0
    swap 5 and 4
  j is 1
  j is 2
    swap 5 and 1
  end 4,5,1,5
i is 1
  begin 4,5,1,5
  j is 0
  j is 1
    swap 5 and 1
  end 4,1,5,5
[4,1,5,5]

The solution is to check changes < 1 or just !changes. That's what's happened upstream in addition to some other optimizations we should eventually grab, but for now I want a very targeted bugfix.

@Bjvanminnen

LGTM. I know Paul did a bunch of work on testing the interpreter. It would be good to (a) see if this changes the pass/fail state of any tests and (b) perhaps add a test for it if not.

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster Nov 8, 2017

Member

I'm working on a regression unit test for this in the code-dot-org repo right now. The tests Paul set up for the interpreter are running in Circle on this PR right now. It runs the test262 EcmaScript Conformance Test Suite, which I suspect will not hit the particular sort edge case we're fixing here, but I also don't feel this merits the effort to set up a second test harness on this repo for custom tests.

Member

islemaster commented Nov 8, 2017

I'm working on a regression unit test for this in the code-dot-org repo right now. The tests Paul set up for the interpreter are running in Circle on this PR right now. It runs the test262 EcmaScript Conformance Test Suite, which I suspect will not hit the particular sort edge case we're fixing here, but I also don't feel this merits the effort to set up a second test harness on this repo for custom tests.

@islemaster islemaster merged commit 64e07a5 into master Nov 8, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@islemaster islemaster deleted the fix-sort branch Nov 8, 2017

@domino14

This comment has been minimized.

Show comment
Hide comment
@domino14

domino14 Nov 15, 2017

why bubble sort

domino14 commented Nov 15, 2017

why bubble sort

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment