Align the ShimMap iterator behavior with native Maps

[kpreisser]

Hi,

this PR adjusts the shimMap that is used by createMap<T>() for IE 11/ES 5 environments (instead of a native Map) to visit values that are added while an iteration or forEach() is running (which is documented for a native Map). Previously, new values during iteration would not be visited, causing problems in IE 11 as described in #26090.
I used the same mechanism for both the forEach call and iterators created with .entries().

Note: With this PR, the shimMap does not support deleting values while an iteration is still running - in this case, when the iterator/forEach loop continues after the delete operation, it will throw an error.

I think it is possible to also support deleting values while an iteration is active using a linked list, but with the caveat that a memory leak could occur if you keep a reference to an non-finished iterator, and then continuously add and delete values from the map. Additionally, as long as there are unfinished iterators (that may or may not be reachable), delete operations on the map would be slow, as they would need to search the iterator key elements.

Update: I have reworked the implementation so that the shimMap should now have the same behavior as a native map for iterators and forEach() loops (see comment below). This means that entries are iterated in insertion order, values added during iteration will be visited, and values removed during iteration (before they were visited) will not be visited.

• Add a test that verifies the new behavior of the shimMap.

Fixes #26090
Fixes #29193

[DanielRosenwasser]

@rbuckton @sheetalkamat @weswigham can you take a look?

[weswigham]

TODO: Add a test that verifies the new behavior of the shimMap (I wasn't sure where to put such a test).

A unit test in the unittests folder. You'd probably need to make shimMap export'd to test it. In runtimes with a real map (aka, everything we CI on) we should probably be testing that it does the same thing as the builtin Map.

[kpreisser]

Hi @weswigham

A unit test in the unittests folder. You'd probably need to make shimMap export'd to test it. In runtimes with a real map (aka, everything we CI on) we should probably be testing that it does the same thing as the builtin Map.

Thank you, I will create a unit test for the shimMap. I think I will even create one that also tests iteration when values are deleted (which will currently fail), because I think I can rework the iterator implementation to match exactly the behavior of a native Map, including visiting entries in insertion order and handling the case when values are deleted while iteration is still running (forEach should behave like using an iterator).

In the current state, this PR handles the case when values are added during iteration, but when values are deleted during iteration, the iterator will throw when continuing it.

In my first comment, I noted this could be handled by using a linked list. I think this can even work without creating a memory leak (as I wrote in the comment) and with better performance. My idea is as follows:

Let's say that we create a map and add some values to it (A, B, C, D):

let map = createMap<string>();
map.set("A", 1);
map.set("B", 2);
map.set("C", 3);
map.set("D", 4);

Then, we create two iterators, and we advanced one of the iterators by two elements:

let iterator1 = map.entries();
let iterator2 = map.entries();

iterator1.next(); // -> A
iterator1.next(); // -> B

Now, the objects will look like this:

Now, delete B:

map.delete("B");

This will change the forward reference of A to point to C (green), and it will change the forward reference of B to point to A, but being marked so that the iterator skips that reference (violet):

Now, delete C:

map.delete("C");

This will change the forward reference of A to point to D, and the reference of C to point to A (skipping reference):

Because C is not reachable, it can be collected by the GC; whereas B (which is removed from the map and the linked list) is still reachable from iterator 1.

Now, add E and delete A:

map.set("E", 5);
map.delete("A");

This means while elements A and B are not in the map and the linked list any more, they are still reachable from the iterator (until its next() method is called).
Note that this shows the memory "leak" I described above: When deleting entries that come directly before the iterator's current position, those cannot be freed since they are still referenced by the iterator until someone calls its next() method. However, this is not a real leak because there cannot be new elements to get in that state as they wil only be appended at the list's tail. Also, if the iterator is thrown away before it is finished, those objects are not reachable anymore and can be freed.

This solves the initial problem I had in the first commit, where the Map had to contain a reference to all active iterators (in order to adjust their indexes when an entry was deleted), so if a iterator was thrown away before iteration finished, the map still had a reference to it. This is no longer the case when using the linked list.
Also, I think the linked list can be set for the lifetime of the map (so it guarantees iteration in insertion order, which is currently not the case), and the elements can be used as values for the map along with a back reference, so delete operations should also be fast.

This should ensure that the iteration of the shimMap works exactly like the one of the "real" Map. I will implement those changes and check if they work correctly.
Thanks!

[kpreisser]

Done: The shimMap should now have the same behavior for iterators and forEach loops as with native maps:

• Entries are visited in insertion order
• New entries added during iteration will be visited
• Entries that are removed during iteration (before being visited) will not be visited

A minor performance drawback is that when calling .clear(), all entries (from the linked list) need to be iterated in order to adjust their references, but I think it is negligible comparing to the better functionality.

[weswigham]

Looks pretty good to me now (test verification is a bit obtuse for my tastes, but it's OK) - @rbuckton ?

[kpreisser]

I have checked that this also fixes #29193.

(test verification is a bit obtuse for my tastes, but it's OK)

Sorry, I didn't have a better idea how to test the iterator behavior 😇

[kpreisser]

Sorry, I think there is still a minor bug in the delete() method. I will look at this tomorrow.

[kpreisser]

OK, fixed: The backward reference of the next entry was not correctly adjusted in delete(). I think it is now good to go.

[rbuckton]

Just one minor issue to address, if you could.

[rbuckton]

Thanks for the contribution!