Use lock for map iteration

[joshwd36]

This Pull Request fixes #1092.

It changes the following:

• Changes the key of OrderedMap to a MapKey, which contains either a key or an empty value.
• Allows locking a map to allow safe iteration.
• When an entry is deleted while the map is locked, it replaces the entry with an empty value to maintain the indexes.
• When the map is unlocked any empty entries are deleted.
• Locks are implemented as a guard object so that they are released even if an error occurs.

Concerns/Questions

• Due to the requirements of the indexmap::Equivalent trait OrderedMap now has the key as a Value rather than a generic parameter. I think this should be fine as I don't think there are other uses of OrderedMap that might require a different key. It may even be possible to implement the Equivalent trait in such a way that it allows the generic parameter to be maintained, but I couldn't find one with a fair amount of experimentation.
• I think the implementation of the MapLock struct is sound, but there might be a better way to do it. I originally just had methods to lock and unlock that would be called before and after iteration, but if an error occured during iteration the unlock function would never be called.
• Are there other ways to trigger this behaviour? Perhaps should all methods on map call lock?

[joshwd36]

I'm not quite sure why that test is failing. It's not something I've touched with my code and I can't reproduce on my Windows machine.

[HalidOdat]

Looks good to me :)

[joshwd36]

Looks good to me :)

Thank you!

One change I had considered but not implemented was instead of using a lock count, and the guard having a GcObject pointing to the map, it might be better to use just an Rc<()> and use the strong count as the lock. Do you think it's worth implementing that?

[HalidOdat]

One change I had considered but not implemented was instead of using a lock count, and the guard having a GcObject pointing to the map, it might be better to use just an Rc<()> and use the strong count as the lock. Do you think it's worth implementing that?

Yes. I think this is much better idea

[HalidOdat]

Due to the requirements of the indexmap::Equivalent trait OrderedMap now has the key as a Value rather than a generic parameter. I think this should be fine as I don't think there are other uses of OrderedMap that might require a different key. It may even be possible to implement the Equivalent trait in such a way that it allows the generic parameter to be maintained, but I couldn't find one with a fair amount of experimentation.

I don't think is a problem, at least for now, If we wanted to implement WeakMap but for this we need to write our own garbage collector to support weak pointers so... it is further away in the future.

I think the implementation of the MapLock struct is sound, but there might be a better way to do it. I originally just had methods to lock and unlock that would be called before and after iteration, but if an error occured during iteration the unlock function would never be called.

It seems sound to me as well, And I think having the struct guard is the easier and cleaner way to do it.

[joshwd36]

One change I had considered but not implemented was instead of using a lock count, and the guard having a GcObject pointing to the map, it might be better to use just an Rc<()> and use the strong count as the lock. Do you think it's worth implementing that?

Yes. I think this is much better idea

Actually, I've just had a go at implementing this and come up against the obstacle that there would be no way to trigger the retain function when the count reached 1 using Rc<()>.

[HalidOdat]

Actually, I've just had a go at implementing this and come up against the obstacle that there would be no way to trigger the retain function when the count reached 1 using Rc<()>.

Hmmm. Yeah that is a problem. In that case lets leave it with this implementation.

[HalidOdat]

I don't think is a problem, at least for now, If we wanted to implement WeakMap but for this we need to write our own garbage collector to support weak pointers so... it is further away in the future.

On more inspection WeakMap does not have functions like values/keys/forEach it only has set/get/has/delete so order and locking is not needed, and we could use std HashMap

[HalidOdat]

Test result	master count	PR count	difference
Total	78,897	78,897	0
Passed	27,954	27,974	+20
Ignored	15,616	15,616	0
Failed	35,327	35,307	-20
Panics	0	0	0
Conformance	35.43%	35.46%	+0.03%

Fixed tests:

test/built-ins/Map/prototype/entries/does-not-have-mapdata-internal-slot-set.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/entries/does-not-have-mapdata-internal-slot-set.js (previously Failed)
test/built-ins/Map/prototype/entries/does-not-have-mapdata-internal-slot.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/entries/does-not-have-mapdata-internal-slot.js (previously Failed)
test/built-ins/Map/prototype/entries/this-not-object-throw.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/entries/this-not-object-throw.js (previously Failed)
test/built-ins/Map/prototype/values/does-not-have-mapdata-internal-slot-set.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/values/does-not-have-mapdata-internal-slot-set.js (previously Failed)
test/built-ins/Map/prototype/values/does-not-have-mapdata-internal-slot.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/values/does-not-have-mapdata-internal-slot.js (previously Failed)
test/built-ins/Map/prototype/values/this-not-object-throw.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/values/this-not-object-throw.js (previously Failed)
test/built-ins/Map/prototype/keys/does-not-have-mapdata-internal-slot-set.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/keys/does-not-have-mapdata-internal-slot-set.js (previously Failed)
test/built-ins/Map/prototype/keys/does-not-have-mapdata-internal-slot.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/keys/does-not-have-mapdata-internal-slot.js (previously Failed)
test/built-ins/Map/prototype/keys/this-not-object-throw.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/keys/this-not-object-throw.js (previously Failed)
test/built-ins/Map/prototype/forEach/iterates-values-deleted-then-readded.js [strict mode] (previously Failed)
test/built-ins/Map/prototype/forEach/iterates-values-deleted-then-readded.js (previously Failed)

[RageKnify]

Hey @joshwd36 you interested on also solving this for Set?

[joshwd36]

Hey @joshwd36 you interested on also solving this for Set?

I should be able to, I'll see if I have any time

[joshwd36]

Hey @joshwd36 you interested on also solving this for Set?

Sorry for the delay, I've done this now.