avm2: Add support for sparse arrays #15491

jarca0123 · 2024-03-08T17:42:05Z

This attempts to implement sparse arrays. There are (as of creating this pull request) big caveats:

~~dense arrays are turned into sparse arrays only if they go beyond current array length and conditions for conversion into sparse arrays from avmplus aren't implemented~~ dense arrays do not implement m_denseStart from avmplus, this might need a custom Vec
it uses BTreeMap, when it probably would be more "correct" to use the DynamicMap implementation
I have no idea if splice works

It does pass tests and also fixes some known failures.

core/src/avm2/array.rs

core/src/avm2/object/array_object.rs

core/src/avm2/array.rs

Lord-McSweeney · 2024-03-14T19:18:46Z

The uses of ArrayStorage::XXX(something, ..) and ArrayStorage::XXX(something, _something_else) can all be replaced with ArrayStorage::XXX(something, _)

core/src/avm2/array.rs

Lord-McSweeney · 2024-03-17T01:25:12Z

Not blocking, but I just realized that several places in ArrayObject do name.parse<usize>() or name.parse<u32>(), but should instead do something like name.parse::<u32>().ok().filter(|s| *s != 4294967295)

core/src/avm2/array.rs

kmeisthax

After reading through most of the array code my main concern is just documenting what the usize parameters do, since that was the biggest thing I was confused about. That's the only thing I feel needs to be fixed before I can merge this.

The splice code seemed straightforward once I puzzled out what it was doing, and it's probably correct. I'm a bit iffy about "drop to splice" as an API but it shouldn't impact correctness in this case. (Remember that memory leaks are safe and that Drop doesn't always run!)

kmeisthax · 2024-04-03T03:47:18Z

core/src/avm2/array.rs

+    index_back: usize,
+}
+
+pub struct ArrayStorageMutableIterator<'a, 'gc> {


If I'm reading the rest of the code correctly, the Mutable variants of the array storage iterator exist primarily so the splice iterators can call into their next/prev methods? It seems awkward, but I'm not aware of a more elegant way to do this...

It exists because I don't know how to implement it without a mutable iterator 😅

core/src/avm2/array.rs

adrian17 · 2024-04-20T18:20:42Z

core/src/avm2/array.rs

+                storage,
+                occupied_count,
+            } => {
+                if storage.len() < (item + 1) {


I think this can be item >= storage.len()

adrian17 · 2024-04-20T18:22:12Z

core/src/avm2/array.rs

+
+    /// Convert the array storage to a sparse representation.
+    fn convert_to_sparse(&mut self) {
+        match self {


Can these be rewritten without the match? Would be a bit shorter

if let ArrayStorage::Dense { storage, .. } = self {

adrian17 · 2024-04-20T18:22:20Z

core/src/avm2/array.rs

+
+    /// Convert the array to a sparse representation if it meets the criteria.
+    fn maybe_convert_to_sparse(&mut self) {
+        match self {


same as above

adrian17 · 2024-04-20T18:24:35Z

core/src/avm2/array.rs

+    /// Convert the array to a dense representation if it meets the criteria.
+    fn maybe_convert_to_dense(&mut self) {
+        match self {
+            ArrayStorage::Dense { .. } => {}


jarca0123 marked this pull request as draft March 9, 2024 17:13

jarca0123 marked this pull request as ready for review March 10, 2024 14:15

evilpie reviewed Mar 10, 2024

View reviewed changes