Skip to content

Conversation

@moulins
Copy link
Contributor

@moulins moulins commented Sep 10, 2024

Replace the use of Rust's standard sort in Array.sort and sortOn by a port of avmplus' QuickSort algorithm. This avoid panics on Rust >=1.81 when Array.sort is called with a non-Ord comparison function, and will always produce the same result as Flash Player.

This also adds a test for AVM1 and AVM2 for "sorting" an array with a random comparison function, that check both the sorted result and the exact sequence of comparisons done by the algorithm. Currently, this test only passes for AVM2; I wasn't able to find which modifications to apply to the AVM1 quicksort to produce the correct results.

@torokati44
Copy link
Member

This will unblock #17812 and #17766, turning the Beta CI checks green.

These tests fail because we do not implement the exact same algorithm
as Flash Player:
- in AVM1, the test produces a different result
- in AVM2, the test panics because Rust's sort doesn't accomodate
  non-Ord comparison functions
…fle-rs#17812

Replace the use of Rust's standard sort in `Array.sort` and `sortOn` by
a port of avmplus' QuickSort algorithm. This avoid panics on Rust >=1.81
when `Array.sort` is called with a non-Ord comparison function, and will
always produce the same result as Flash Player.
@torokati44 torokati44 enabled auto-merge (rebase) September 11, 2024 19:47
@torokati44 torokati44 merged commit 725d887 into ruffle-rs:master Sep 11, 2024
@moulins moulins deleted the avm2-qsort branch September 11, 2024 22:09
@Voultapher
Copy link

I wonder if this change is a good fit for running arbitrary user-code. It's trivial to hit quadratic run-time, e.g:

let len = 1_000_000;
let mut v = (0..len as i32).map(|val| val % 2).collect::<Vec<i32>>();
let _ = qsort(&mut v, &mut |a, b| -> Result<Ordering, ()> { Ok(a.cmp(b)) });

I guess if you are going for bug-for-bug compatibility with the avmplus code this is the correct choice.

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.

5 participants