Hello, I've updated the PR [1] with the following changes: - made the Map.Entry instance returned by PointMap.getEntry() support the setValue() method for all spaces and dimensions - created an abstract base class to share code between the 1D PointMap implementations - fixed a bug in the 1D spherical point map remove() method - added additional unit tests
Supporting setValue() in the multidimensional cases was trivial since I simply returned the actual SimpleEntry instance stored in the map instead of an immutable wrapper instance. For the 1D cases, I made getEntry() return a wrapper instance that calls Map.replace() on the underlying TreeMap when the value is changed. This forces another key look up but I believe that the small overhead is worth it in order to provide a consistent PointMap API. In any case, the overall performance is not affected since calling put() or replace() is the only method available to modify a TreeMap entry (outside of using the entrySet). Let me know what you think. Regards, Matt [1] https://github.com/apache/commons-geometry/pull/194 On Wed, Mar 16, 2022 at 12:30 PM Gilles Sadowski <gillese...@gmail.com> wrote: > > Hi. > > Le mer. 16 mars 2022 à 15:42, Matt Juntunen > <matt.a.juntu...@gmail.com> a écrit : > > > > Hello, > > > > > I suggest to carefully consider whether to return a "SimpleEntry" > > (and prominently note that any sort of concurrent modification is > > a caller responsibility). > > > > I see what you mean and I think that would be a good idea. However, > > the sticking point is that the 1D implementations for both Euclidean > > and spherical space internally use the JDK's TreeMap class to store > > entries, due to its superior performance when compared to the > > AbstractBucketPointMap class used for other dimensions. TreeMap > > returns immutable Map.Entry instances from its entry-access methods > > (e.g., ceilingEntry, floorEntry), > > The apidocs[1] state: > ---CUT--- > Map.Entry<K,V> ceilingEntry(K key) > Returns a key-value mapping associated with the least key greater > than or equal to the given key, or null if there is no such key. > ---CUT--- > > > so there is not a straightforward > > way for us to implement the behavior you propose for these dimensions. > > AFAICT, (im)mutability of the returned entry is not part of the > JDK-mandated API. > So, assuming that the behaviour is implementation-dependent, > it can be chosen to be different for different dimensions on the > basis of which behaviour is most "natural" for applications. > > > The options I see are: > > 1. Have all returned entries be immutable (the current behavior). > > 2. Return specialized Map.Entry implementations for the 1D cases that > > call the "put" method on the underlying map when "setValue" is called. > > > > Option #2 seems less than ideal so unless there is another approach > > that I'm missing, I vote for #1. > > I agree that the situation is somewhat unsatisfying. But, as said, I'd > favour #1 only if there were an actual security promise. Otherwise, > immutability is a false claim. > Unless I'm mistaken, calling "put" in order to update the "value" is > necessarily less performant than calling "setValue" (map search in > the former, no-op in the latter). > > Regards, > Gilles > > [1] > https://docs.oracle.com/javase/8/docs/api/java/util/TreeMap.html#ceilingEntry-K- > > > Regards, > > Matt > > > > > > On Wed, Mar 16, 2022 at 9:48 AM Gilles Sadowski <gillese...@gmail.com> > > wrote: > > > > > > Hi. > > > > > > Le mer. 16 mars 2022 à 03:17, Matt Juntunen > > > <matt.a.juntu...@gmail.com> a écrit : > > > > > > > > Hello, > > > > > > > > I've made the following changes to the PR: > > > > - removed the "resolveKey" method from PointMap > > > > - renamed PointMap.resolveEntry to PointMap.getEntry and > > > > PointSet.resolve to PointSet.get > > > > - added an entry on PointMap/PointSet to the user guide > > > > - addressed Github comments (thanks, Bruno!) > > > > > > > > I ran some performance tests regarding the immutable entry instance > > > > created in the PointMap.getEntry method and there seems to be no > > > > impact. > > > > > > > > > Furthermore, what is actually meant here by "immutable > > > > instance" (since the "value" could be mutable without the > > > > map being aware of the fact)? > > > > > > > > It is immutable in that the object reference used as the entry value > > > > cannot be changed. This reference could point to a mutable object. > > > > This is the same behavior as other Map implementations. > > > > > > I don't see that "reference immutability" is mandated by the > > > "Map" interface (see e.g. [1]). > > > > > > I've noted many times that I generally favour (true) immutability: > > > It makes much sense for "small" data-structures (e.g. for future > > > potential optimizations[2]). > > > > > > However, the "cloud of points" data-structure is at the opposite > > > of the spectrum from this POV: It is intended to contain a large > > > number of points whose "key" should indeed be (truly) immutable > > > but whose value would likely need to be mutable for many actual > > > use cases. > > > If a "SimpleImmutableEntry" is returned, then in order to modify > > > the map's "value" contents, one has to (IIUC) > > > * retrieve the entry, > > > * create a new value, > > > * call "put" (on the map) > > > rather than > > > * retrieve the entry > > > * call "setValue" (on the entry). > > > So we have a somewhat crippled API that does not bring any > > > advantage since reference immutability doesn't provide any > > > security to the map's user (any other caller who is being passed > > > the same map, is able to change its contents anyways). > > > > > > I suggest to carefully consider whether to return a "SimpleEntry" > > > (and prominently note that any sort of concurrent modification is > > > a caller responsibility). > > > > > > Regards, > > > Gilles > > > > > > [1] > > > https://docs.oracle.com/javase/8/docs/api/java/util/HashMap.html#entrySet-- > > > [2] > > > https://cr.openjdk.java.net/~briangoetz/valhalla/sov/02-object-model.html > > > > > > >>> [...] > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
