rishabhdaim commented on PR #2178: URL: https://github.com/apache/jackrabbit-oak/pull/2178#issuecomment-2721547723
> I think this replacement is too dangerous. It changes significantly the semantics and performance characteristics. > > * Changes mutability: LinkedHashSet is not immutable. > > * Changes performance: the sets created by ImmutableSet.copyOf() are highly optimized. There are special implementations for empty and one-element sets. And the implementation for general sized sets is also optimised, taking advantage of the set being immutable. It stores the elements in an array, which is more compact and faster to access than in a LinkedHashSet. > > > This might have unintended consequences in performance and even in correctness. I believe that these comments are given on the assumption that all the changes are mechanical which is not true. I checked the usage before making this change. I also checked the previous PR, where we removed the `ImmutableSet.copyOf` operation and replaced it with `HashSet`, and IMO, that's what caused the failures because Guava maintains the insertion order while HashSet doesn't. This is the reason why I have chosen `LinkedHashSet` now. Second, about the performance, we are only using the `ImmutableSet.copyOf(Iterables/Iterator ...)` variant, and that also takes `O(n)` in Guava's implementation, and since it maintains an additional `Hashtable`, it uses more space. In my implementation, I am simply creating a `LinkedHashSet`, and the time complexity is the same as that of Guava. Regarding Immutability, there is nothing in JDK or Apache's `commons-collections4` that provides the drop-in replacement of `ImmutableSet` of Guava. `Set.of` again, changes the iteration order and thus we again risk the previous issue. We could overcome this (case by case basis) with either `Collections.unmodifiableSet` from JDK and `UnmodifiableSet.unmodifiableSet(set)` from `commons-collections4` but that won't be necessary in the majority of the cases. But just to remove any doubt, I would again do a thorough review of all the cases and wrap it inside an unmodifiable wrapper in case of any doubt. cc @nfsantos @reschke -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
