Optimize CopyOnWriteArrayList to use scala.Array on Wasm #5211
Conversation
e3c20ce to
b145cc0
Compare
|
oops, some tests failed |
| protected def innerPush(elem: E): Unit = | ||
| inner.push(elem) | ||
| protected def innerPush(elem: E): Unit = { | ||
| ensureCapacity(size() + 1) |
There was a problem hiding this comment.
In CopyOnWriteArrayList, we should keep the regular notion of capacity (without the + 1). It made sense in PriorityQueue because the generic implementation avoids index 0 anyway. But in CopyOnWriteArrayList, avoiding 0 is a Wasm-only thing, due to storing the length.
We should concentrate all those + 1s and - 1s inside JArrayImpl.
| inner = innerImpl.resize(inner, minCapacity) | ||
| } | ||
| } | ||
| // We ignore this in JS as js.Array doesn't support explicit pre-allocation |
There was a problem hiding this comment.
Like in PriorityQueue, we can replace the Wasm-only capacity() and resize() by a generic, virtual ensureCapacity() (which happens to do nothing in JSArrayImpl).
| } | ||
|
|
||
| private[concurrent] sealed abstract class InnerArrayImpl { | ||
| type Repr <: AnyRef |
b145cc0 to
bb0c77b
Compare
|
Now this change follows what Also, made a rough benchmark and verified that Wasm performance has improved, while JS remains unchanged
|
| */ | ||
|
|
||
| // package private so that `protected def innerSnapshot` can access this field. | ||
| private[concurrent] val innerImpl: InnerArrayImpl = |
There was a problem hiding this comment.
| private[concurrent] val innerImpl: InnerArrayImpl = | |
| private[concurrent] val innerImpl: InnerArrayImpl = { |
| v.splice(index, 0, e.asInstanceOf[AnyRef]) | ||
| v | ||
| } | ||
| @inline def add[E](v: Repr[E], index: Int, items: Collection[_ <: E]): Repr[E] = { |
There was a problem hiding this comment.
Given that the methods have significant bodies, in this one, let's have real blank lines between each method, as is our regular coding style.
| @inline def add[E](v: Repr[E], index: Int, items: Collection[_ <: E]): Repr[E] = { | ||
| val innerIdx = index + 1 | ||
| val size = length(v) | ||
| val newArr = ensureCapacity(v, size + items.size()) |
There was a problem hiding this comment.
There are several calls to items.size(). Consider storing that first into a val.
| val size = length(v) | ||
| val removed = v(innerIdx) | ||
| System.arraycopy(v, innerIdx + 1, v, innerIdx, size - innerIdx) | ||
| v(size) = null.asInstanceOf[AnyRef] // free reference for GC |
There was a problem hiding this comment.
| v(size) = null.asInstanceOf[AnyRef] // free reference for GC | |
| v(size) = null // free reference for GC |
| private class CopyOnWriteArrayListIterator[E](arraySnapshot: innerImpl.Repr[E], | ||
| i: Int, start: Int, end: Int) extends AbstractRandomAccessListIterator[E](i, start, end) { |
There was a problem hiding this comment.
| private class CopyOnWriteArrayListIterator[E](arraySnapshot: innerImpl.Repr[E], | |
| i: Int, start: Int, end: Int) extends AbstractRandomAccessListIterator[E](i, start, end) { | |
| private class CopyOnWriteArrayListIterator[E]( | |
| arraySnapshot: innerImpl.Repr[E], i: Int, start: Int, end: Int) | |
| extends AbstractRandomAccessListIterator[E](i, start, end) { |
There was a problem hiding this comment.
Was it actually necessary to move this inside the companion object? If not, consider leaving it out to keep a smaller diff.
There was a problem hiding this comment.
It is not, moved back to outside of the companion object :)
bb0c77b to
cdc5062
Compare
This commit optimizes `CopyOnWriteArrayList` to use a platform-specific internal array (scala.Array on Wasm, and js.Array on JS), similar to what was done for `ArrayList`. On WebAssembly, it now uses a `scala.Array[AnyRef]` for its internal storage. This improves performance by avoiding JS interop overhead for array operations. On JavaScript, it continues to use `js.Array`. InnerArrayImpl abstracts away the platform-specific details from the main class. To avoid keeping track the length of array in the main class on Wasm, we store the length of the array at the index 0 of the array.
cdc5062 to
718d3ec
Compare
This commit optimizes
CopyOnWriteArrayListto use a platform-specific internal array (scala.Array on Wasm, and js.Array on JS), similar to what was done forArrayList.On WebAssembly, it now uses a
scala.Array[AnyRef]for its internal storage. This improves performance by avoiding JS interop overhead for array operations. On JavaScript, it continues to usejs.Array. InnerArrayImpl abstracts away the platform-specific details from the main class.To avoid keeping track the length of array in the main class on Wasm, we store the length of the array at the index 0 of the array.