blerer commented on code in PR #15457:
URL: https://github.com/apache/lucene/pull/15457#discussion_r2578263716
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfo.java:
##########
@@ -675,7 +674,7 @@ public synchronized String putAttribute(String key, String
value) {
String oldValue = newMap.put(key, value);
// This needs to be thread-safe as multiple threads may be updating
(different) attributes
// concurrently due to concurrent merging.
- attributes = Collections.unmodifiableMap(newMap);
+ attributes = Map.copyOf(newMap);
Review Comment:
The idea was to align the code of `FieldInfo` with the one of `SegmentInfo`.
Currently `SegmentInfo` is using `ImmutableCollections.AbstractImmutableMap`
(through `Map.copyOf`) whereas `FieldInfo` is using `UnmodifiableMap` (through
`Collections.unmodifiable`). Originally `SegmentInfo` was doing some defensive
copies using `Collections.unmodifiableMap(new
HashMap<>(Objects.requireNonNull(diagnostics)));` but they have been replaced
by `Maps.copyOf` in #649
##########
lucene/CHANGES.txt:
##########
@@ -69,6 +69,8 @@ Improvements
* GITHUB#15124: Use RamUsageEstimator to calculate size for non-accountable
queries. (Sagar Upadhyaya)
+* GITHUB#15453: Avoid unnecessary sorting and instantiations in
readMapOfStrings. (Benjamin Lerer)
Review Comment:
In `SegmentInfo` the maps were not ordered before the change because te use
of `Map.copyOf`, in the constructor, was discarding the `TreeMap` ordering. A
similar thing was happening for the `TreeSet`. Its elements were converted and
added to a `HashSet`. Only `FieldInfo` `attributes` was somehow retaining the
order has long as no new attibutes were added. Of course that was only valid if
you add less than 10 elements within the maps.
##########
lucene/core/src/java/org/apache/lucene/store/DataInput.java:
##########
@@ -248,17 +243,17 @@ public DataInput clone() {
public Map<String, String> readMapOfStrings() throws IOException {
int count = readVInt();
if (count == 0) {
- return Collections.emptyMap();
+ return Map.of();
} else if (count == 1) {
- return Collections.singletonMap(readString(), readString());
+ return Map.of(readString(), readString());
Review Comment:
Currently the `readString` method cannot return a `null` `String` and
`SegmentInfo` convert the Maps through `Map.copyOf` which enforce the same
check, just later on. Also `writeString` do not support `null` values.
Therefore, it does not look like `null` `String` are supported by the code.
That being said I understand the concern. The choice of using immutable maps
(created throught `Map.of` and `Map.copyOf`) and set was to align with what was
done within `SegmentInfo` and avoid an extra instantiation as a `Map.copyOf` of
a immutable map is a noop.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
