Inconsistent behavior between shift / pop vs clear / remove / removeOne / splice

[dnlmlr]

While looking through the sourcecode I noticed that the shift and pop functions actually set the removed element to undefined, while all other functions don't. It is the most obvious with the clear function where it just sets this._head = this._tail = 0;. Sure it states that the capacity remains unchanged, but that is not the problem. As long as the elements are not set to undefined, the array holds references to the elements in memory, preventing the garbage collector from freeing said memory.

As an example (ignoring memory other then the data objects):

• Push 10x 100MB Objects into the queue (for example large strings, or other data buffers, or at a smaller scale very large JSON objects)
• Now the process will use 1000MB of memory
• Shift 10x
• Now the process will use 0MB (the array stores only references, so an array with 10x undefined doesn't take use much memory itsef)
• Push 10x 100MB Objects again
• Process will use 1000MB of memory again
• Call clear
• Process still uses 1000MB of memory, because the array still holds the references

In both cases the capacity of the internal array will remain unchanged, but with clear it creates kind of a leak.

I don't think either behavior is wrong, but the undocumented inconsistency is not good imo. If it is ok to leak the deleted cells, the same behavior could be used in shift / pop and likely improve performance in those functions. If it is considered to be not ok in shift / pop, it shouldn't be ok for the other functions either.

Edit: The builtin javascript Array functions splice and length=x do in fact seem to free the references of all removed elements

[Salakar]

We should probably match the behaviour of the built-in js array functions across the board for consistency I think, thoughts?

I can't remember why it was done like this, I wrote this so long ago, but it probably is just something I missed. Would you be willing to send a PR for this also?

[dnlmlr]

Matching the built-in array functions sounds sane.

I took another look at the functions and it seems that my initial comment was wrong since I missed the void 0 which are used as undefined at some points. removeOne actually does set the deleted element to undefined, so that's working as expected.

splice and remove try to set the elements to undefined, but when removing more than one at a time, they seem to miss exactly 1 element at the edge. That seems to me like a bug, but I won't even pretend to understand what is going on in the implementations of these functions. So I won't be able to change anything in there.

When running this code and looking at the internal buffer, the element at index 0 is still there, but index 1 was deleted. When deleting more elements and / or from other locations, there will always be 1 deleted element left over that is not set to undefined.

let q = new Denque([0, 1, 2, 3, 4, 5, 6]);
console.log(q._list); // [ 0, 1, 2, 3, 4, 5, 6, <1 empty item> ]
console.log(q.toArray()); // [0, 1, 2, 3, 4, 5, 6]

q.splice(0, 2)

console.log(q._list); // [ 0, undefined, 2, 3, 4, 5, 6, <1 empty item> ]
console.log(q.toArray()); // [ 2, 3, 4, 5, 6 ]

clear behaves exactly as I described in my original comment. This would be an easy fix: Just overwrite the buffer _list with a new one. That way the garbage collector will take care of the elements and the queue is empty again.

[Salakar]

I see, let's at least fix clear if we can. For splice and remove that does indeed seem like a bug but I'd need some time to investigate those

[dnlmlr]

I saw this message coincidentally before turning off the pc for today, so I quickly made another PR (#47) that should fix the issue for clear