You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "bruno-roustant (via GitHub)" <gi...@apache.org> on 2024/04/29 13:49:07 UTC
[PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
bruno-roustant opened a new pull request, #13327:
URL: https://github.com/apache/lucene/pull/13327
### Description
Two goals:
1- Reduce the memory usage of field maps when there are many fields.
FieldInfos keeps an array of FieldInfo of the size on the max field number, which can be sparse if the number is high while a few fields are indexed.
Use a primitive IntObjectHashMap to reduce the memory usage compared to an HashMap in BlockTree TermsReader.
2- Add new IntObjectHashMap in the existing small hppc fork. Leverage this PR to show an example use-case. It hopefully can be reused later for other use-cases.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1583142363
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -81,30 +81,13 @@ public FieldInfos(FieldInfo[] infos) {
String softDeletesField = null;
String parentField = null;
- int size = 0; // number of elements in byNumberTemp, number of used array slots
- FieldInfo[] byNumberTemp = new FieldInfo[10]; // initial array capacity of 10
+ byName = new HashMap<>((int) (infos.length / 0.75f) + 1);
for (FieldInfo info : infos) {
if (info.number < 0) {
throw new IllegalArgumentException(
"illegal field number: " + info.number + " for field " + info.name);
}
- size = info.number >= size ? info.number + 1 : size;
Review Comment:
Instead of always building an array of FieldInfo of the size of the max field number + 1, we have some new code below to create either an ArrayFieldInfoByNumber or a MapFieldInfoByNumber. This way we don't create a sparse array if the max field number is high compared to the actual number of fields in this FieldInfos.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1585022423
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -156,15 +139,17 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- List<FieldInfo> valuesTemp = new ArrayList<>(infos.length);
- byNumber = new FieldInfo[size];
- for (int i = 0; i < size; i++) {
- byNumber[i] = byNumberTemp[i];
- if (byNumberTemp[i] != null) {
- valuesTemp.add(byNumberTemp[i]);
- }
- }
- values = Collections.unmodifiableCollection(valuesTemp);
+ FieldInfo[] sortedFieldInfos = ArrayUtil.copyOfSubArray(infos, 0, infos.length);
+ Arrays.sort(sortedFieldInfos, (fi1, fi2) -> Integer.compare(fi1.number, fi2.number));
+ int maxFieldNumber = infos.length == 0 ? -1 : sortedFieldInfos[infos.length - 1].number;
+ // If there are many fields and the max field number is greater than twice the number
+ // of fields, then a map structure is more compact to store the by-number mapping.
+ byNumber =
+ maxFieldNumber >= 2 * infos.length && maxFieldNumber >= 32
+ ? new MapFieldInfoByNumber(infos)
+ : new ArrayFieldInfoByNumber(infos, maxFieldNumber);
+ // The iteration of FieldInfo is ordered by ascending field number.
+ values = Collections.unmodifiableCollection(Arrays.asList(sortedFieldInfos));
Review Comment:
List.of() makes a copy as it considers the input as an "untrusted array". Here we don't copy, just wrap. Actually we could keep just Arrays.asList(sortedFieldInfos) since we own it privately, so we know we don't modify it (only iterator(), which does not support removal for Arrays.asList).
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1597469133
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -139,17 +148,38 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- FieldInfo[] sortedFieldInfos = ArrayUtil.copyOfSubArray(infos, 0, infos.length);
- Arrays.sort(sortedFieldInfos, (fi1, fi2) -> Integer.compare(fi1.number, fi2.number));
- int maxFieldNumber = infos.length == 0 ? -1 : sortedFieldInfos[infos.length - 1].number;
- // If there are many fields and the max field number is greater than twice the number
- // of fields, then a map structure is more compact to store the by-number mapping.
- byNumber =
- maxFieldNumber >= 2 * infos.length && maxFieldNumber >= 32
- ? new MapFieldInfoByNumber(infos)
- : new ArrayFieldInfoByNumber(infos, maxFieldNumber);
- // The iteration of FieldInfo is ordered by ascending field number.
- values = Collections.unmodifiableCollection(Arrays.asList(sortedFieldInfos));
+ if (fieldNumberStrictlyAscending && maxFieldNumber == infos.length - 1) {
+ // The input FieldInfo[] contains all fields numbered from 0 to infos.length - 1 and they are
+ // sorted, use it directly. This is an optimization when reading a segment with all fields
+ // since the FieldInfo[] is sorted.
+ byNumber = infos; // We could copy the input array, but do we need to?
+ values = Arrays.asList(byNumber);
+ } else {
+ byNumber = new FieldInfo[maxFieldNumber + 1];
+ for (FieldInfo fieldInfo : infos) {
+ FieldInfo previous = byNumber[fieldInfo.number];
Review Comment:
Thanks. "previous" wasn't wrong, just ambiguous. I like "existing"; another possible name is "old".
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1597449679
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -139,17 +148,38 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- FieldInfo[] sortedFieldInfos = ArrayUtil.copyOfSubArray(infos, 0, infos.length);
- Arrays.sort(sortedFieldInfos, (fi1, fi2) -> Integer.compare(fi1.number, fi2.number));
- int maxFieldNumber = infos.length == 0 ? -1 : sortedFieldInfos[infos.length - 1].number;
- // If there are many fields and the max field number is greater than twice the number
- // of fields, then a map structure is more compact to store the by-number mapping.
- byNumber =
- maxFieldNumber >= 2 * infos.length && maxFieldNumber >= 32
- ? new MapFieldInfoByNumber(infos)
- : new ArrayFieldInfoByNumber(infos, maxFieldNumber);
- // The iteration of FieldInfo is ordered by ascending field number.
- values = Collections.unmodifiableCollection(Arrays.asList(sortedFieldInfos));
+ if (fieldNumberStrictlyAscending && maxFieldNumber == infos.length - 1) {
+ // The input FieldInfo[] contains all fields numbered from 0 to infos.length - 1 and they are
+ // sorted, use it directly. This is an optimization when reading a segment with all fields
+ // since the FieldInfo[] is sorted.
+ byNumber = infos; // We could copy the input array, but do we need to?
+ values = Arrays.asList(byNumber);
+ } else {
+ byNumber = new FieldInfo[maxFieldNumber + 1];
+ for (FieldInfo fieldInfo : infos) {
+ FieldInfo previous = byNumber[fieldInfo.number];
Review Comment:
Yes, existing, to check we have no duplicates. Let's rename it "existing" for clarity.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1585022423
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -156,15 +139,17 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- List<FieldInfo> valuesTemp = new ArrayList<>(infos.length);
- byNumber = new FieldInfo[size];
- for (int i = 0; i < size; i++) {
- byNumber[i] = byNumberTemp[i];
- if (byNumberTemp[i] != null) {
- valuesTemp.add(byNumberTemp[i]);
- }
- }
- values = Collections.unmodifiableCollection(valuesTemp);
+ FieldInfo[] sortedFieldInfos = ArrayUtil.copyOfSubArray(infos, 0, infos.length);
+ Arrays.sort(sortedFieldInfos, (fi1, fi2) -> Integer.compare(fi1.number, fi2.number));
+ int maxFieldNumber = infos.length == 0 ? -1 : sortedFieldInfos[infos.length - 1].number;
+ // If there are many fields and the max field number is greater than twice the number
+ // of fields, then a map structure is more compact to store the by-number mapping.
+ byNumber =
+ maxFieldNumber >= 2 * infos.length && maxFieldNumber >= 32
+ ? new MapFieldInfoByNumber(infos)
+ : new ArrayFieldInfoByNumber(infos, maxFieldNumber);
+ // The iteration of FieldInfo is ordered by ascending field number.
+ values = Collections.unmodifiableCollection(Arrays.asList(sortedFieldInfos));
Review Comment:
List.of() makes a copy as it considers the input as an "untrusted array". Here we don't copy, just wrap. Actually we could keep just Arrays.asList(sortedFieldInfos) since we own it privately, so we know we don't modify it (only iterator()).
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1584722315
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/blocktree/Lucene90BlockTreeTermsReader.java:
##########
@@ -113,7 +114,8 @@ public final class Lucene90BlockTreeTermsReader extends FieldsProducer {
// produce DocsEnum on demand
final PostingsReaderBase postingsReader;
- private final Map<String, FieldReader> fieldMap;
+ private final FieldInfos fieldInfos;
+ private final IntObjectHashMap<FieldReader> fieldMap;
Review Comment:
FWIW this is what Lucene90PointsReader does today as well.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1587798999
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -156,15 +148,38 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- List<FieldInfo> valuesTemp = new ArrayList<>(infos.length);
- byNumber = new FieldInfo[size];
- for (int i = 0; i < size; i++) {
- byNumber[i] = byNumberTemp[i];
- if (byNumberTemp[i] != null) {
- valuesTemp.add(byNumberTemp[i]);
+ if (fieldNumberStrictlyAscending && maxFieldNumber == infos.length - 1) {
+ // The input FieldInfo[] contains all fields numbered from 0 to infos.length - 1 and they are
+ // sorted, use it directly. This is an optimization when reading a segment with all fields
+ // since the FieldInfo[] is sorted.
+ byNumber = infos; // We could copy the input array, but do we need to?
Review Comment:
Here it seems to me we can use directly the input array. Do you think we should copy it for safety?
This input array is created by the readers and passed as parameter to the constructor.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1585048996
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/blocktree/Lucene90BlockTreeTermsReader.java:
##########
@@ -113,7 +114,8 @@ public final class Lucene90BlockTreeTermsReader extends FieldsProducer {
// produce DocsEnum on demand
final PostingsReaderBase postingsReader;
- private final Map<String, FieldReader> fieldMap;
+ private final FieldInfos fieldInfos;
+ private final IntObjectHashMap<FieldReader> fieldMap;
Review Comment:
Nice. It could benefit from the IntObjectHashMap.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on PR #13327:
URL: https://github.com/apache/lucene/pull/13327#issuecomment-2090722649
So I did some benchmarking.
As I could not reproduce the sparse byNumber mapping, I prefer to remove the code for the primitive map in FieldInfos.
At the same time I could evaluate the time spent by the current FieldInfos code to build the byNumber and values arrays. When we read a segment, the reader provides the FieldInfo[] input always sorted in ascending order for number. So the code that grows byNumberTemp has to grow multiple times when there a many fields. Same for the hashmap initialized with default capacity.
This latest commit changes the way byNumber is built to only allocate one array, or even proposes to use directly the input FieldInfo[] if it is full. This constructor becomes 40% faster than before, even more when the GC is busy, as it allocates less arrays.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1583136349
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/blocktree/Lucene90BlockTreeTermsReader.java:
##########
@@ -113,7 +114,8 @@ public final class Lucene90BlockTreeTermsReader extends FieldsProducer {
// produce DocsEnum on demand
final PostingsReaderBase postingsReader;
- private final Map<String, FieldReader> fieldMap;
+ private final FieldInfos fieldInfos;
+ private final IntObjectHashMap<FieldReader> fieldMap;
Review Comment:
This PR proposes to leverage the existing field-name -> FieldInfo map in FieldInfos to not repeat the ref to the field name strings here. Instead use the field number (specific to the FieldInfos) as key, so that we can use a compact primitive map.
Then below, in terms(String fieldName), we can use FieldInfos.fieldInfo(String fieldName) as a first mapping to the field number, and then use this compact map to get the Terms.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1584952051
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -81,30 +81,13 @@ public FieldInfos(FieldInfo[] infos) {
String softDeletesField = null;
String parentField = null;
- int size = 0; // number of elements in byNumberTemp, number of used array slots
- FieldInfo[] byNumberTemp = new FieldInfo[10]; // initial array capacity of 10
+ byName = new HashMap<>((int) (infos.length / 0.75f) + 1);
Review Comment:
see `org.apache.lucene.util.CollectionUtil#newHashMap` instead
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -821,4 +803,64 @@ FieldInfos finish() {
return new FieldInfos(byName.values().toArray(new FieldInfo[byName.size()]));
}
}
+
+ private interface FieldInfoByNumber {
Review Comment:
or just use `IntFunction` ?
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -156,15 +139,17 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- List<FieldInfo> valuesTemp = new ArrayList<>(infos.length);
- byNumber = new FieldInfo[size];
- for (int i = 0; i < size; i++) {
- byNumber[i] = byNumberTemp[i];
- if (byNumberTemp[i] != null) {
- valuesTemp.add(byNumberTemp[i]);
- }
- }
- values = Collections.unmodifiableCollection(valuesTemp);
+ FieldInfo[] sortedFieldInfos = ArrayUtil.copyOfSubArray(infos, 0, infos.length);
+ Arrays.sort(sortedFieldInfos, (fi1, fi2) -> Integer.compare(fi1.number, fi2.number));
+ int maxFieldNumber = infos.length == 0 ? -1 : sortedFieldInfos[infos.length - 1].number;
+ // If there are many fields and the max field number is greater than twice the number
+ // of fields, then a map structure is more compact to store the by-number mapping.
+ byNumber =
+ maxFieldNumber >= 2 * infos.length && maxFieldNumber >= 32
+ ? new MapFieldInfoByNumber(infos)
+ : new ArrayFieldInfoByNumber(infos, maxFieldNumber);
+ // The iteration of FieldInfo is ordered by ascending field number.
+ values = Collections.unmodifiableCollection(Arrays.asList(sortedFieldInfos));
Review Comment:
nowadays, do `List.of(sortedFieldInfos)` rather than this double-layer referring to two other classes
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1597498694
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -156,15 +151,38 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- List<FieldInfo> valuesTemp = new ArrayList<>(infos.length);
- byNumber = new FieldInfo[size];
- for (int i = 0; i < size; i++) {
- byNumber[i] = byNumberTemp[i];
- if (byNumberTemp[i] != null) {
- valuesTemp.add(byNumberTemp[i]);
+ if (fieldNumberStrictlyAscending && maxFieldNumber == infos.length - 1) {
+ // The input FieldInfo[] contains all fields numbered from 0 to infos.length - 1, and they are
+ // sorted, use it directly. This is an optimization when reading a segment with all fields
+ // since the FieldInfo[] is sorted.
+ byNumber = infos;
+ values = Arrays.asList(byNumber);
+ } else {
+ byNumber = new FieldInfo[maxFieldNumber + 1];
+ for (FieldInfo fieldInfo : infos) {
+ FieldInfo existing = byNumber[fieldInfo.number];
+ if (existing != null) {
+ throw new IllegalArgumentException(
+ "duplicate field numbers: "
+ + existing.name
+ + " and "
+ + fieldInfo.name
+ + " have: "
+ + fieldInfo.number);
+ }
+ byNumber[fieldInfo.number] = fieldInfo;
+ }
+ if (maxFieldNumber == infos.length - 1) {
+ // No fields are missing, use byNumber.
+ values = Arrays.asList(byNumber);
+ } else {
+ // The below code is faster than Arrays.stream(byNumber).filter(Objects::nonNull).toList(),
+ // mainly when the input FieldInfo[] is sorted, when reading a segment.
Review Comment:
Here maxFieldNumber > infos.length - 1, so some fields are missing in byNumber (byNumber[fieldNumber] == null). We must not return those null FieldInfos in values.iterator().
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on PR #13327:
URL: https://github.com/apache/lucene/pull/13327#issuecomment-2085548260
Very interesting, I didn't know this history of the FieldInfos.
I ended up analyzing the FieldInfos after we saw the time and memory usage for the byNumber array when there are many fields.
I think the time comes from the ArrayUtil.grow(byNumberTemp, info.number + 1), when the array has to grow and copy multiple times.
For the memory part, it was more a guess since we saw large FieldInfo[] that could be explained if the max info.number was high. But I have no metrics to share unfortunately.
If the decision was already taken to stay on a dense array, then I can remove the primitive map from this PR. But I would like to keep the creation and the sorting of the array at the end of the loop, as I think it is faster.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1597484741
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -139,17 +148,38 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- FieldInfo[] sortedFieldInfos = ArrayUtil.copyOfSubArray(infos, 0, infos.length);
- Arrays.sort(sortedFieldInfos, (fi1, fi2) -> Integer.compare(fi1.number, fi2.number));
- int maxFieldNumber = infos.length == 0 ? -1 : sortedFieldInfos[infos.length - 1].number;
- // If there are many fields and the max field number is greater than twice the number
- // of fields, then a map structure is more compact to store the by-number mapping.
- byNumber =
- maxFieldNumber >= 2 * infos.length && maxFieldNumber >= 32
- ? new MapFieldInfoByNumber(infos)
- : new ArrayFieldInfoByNumber(infos, maxFieldNumber);
- // The iteration of FieldInfo is ordered by ascending field number.
- values = Collections.unmodifiableCollection(Arrays.asList(sortedFieldInfos));
+ if (fieldNumberStrictlyAscending && maxFieldNumber == infos.length - 1) {
+ // The input FieldInfo[] contains all fields numbered from 0 to infos.length - 1 and they are
+ // sorted, use it directly. This is an optimization when reading a segment with all fields
+ // since the FieldInfo[] is sorted.
+ byNumber = infos; // We could copy the input array, but do we need to?
+ values = Arrays.asList(byNumber);
+ } else {
+ byNumber = new FieldInfo[maxFieldNumber + 1];
+ for (FieldInfo fieldInfo : infos) {
+ FieldInfo previous = byNumber[fieldInfo.number];
+ if (previous != null) {
+ throw new IllegalArgumentException(
+ "duplicate field numbers: "
+ + previous.name
+ + " and "
+ + fieldInfo.name
+ + " have: "
+ + fieldInfo.number);
+ }
+ byNumber[fieldInfo.number] = fieldInfo;
+ }
+ if (maxFieldNumber == infos.length - 1) {
+ // No fields are missing, use byNumber.
+ values = Arrays.asList(byNumber);
+ } else {
+ // The below code is faster than Arrays.stream(byNumber).filter(Objects::nonNull).toList(),
+ // mainly when the input FieldInfo[] is sorted, when reading a segment.
+ FieldInfo[] sortedFieldInfos = ArrayUtil.copyOfSubArray(infos, 0, infos.length);
Review Comment:
Don't even need to copy it as we've given ourselves permission to take ownership of the input array; to manipulate it.
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -156,15 +151,38 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- List<FieldInfo> valuesTemp = new ArrayList<>(infos.length);
- byNumber = new FieldInfo[size];
- for (int i = 0; i < size; i++) {
- byNumber[i] = byNumberTemp[i];
- if (byNumberTemp[i] != null) {
- valuesTemp.add(byNumberTemp[i]);
+ if (fieldNumberStrictlyAscending && maxFieldNumber == infos.length - 1) {
+ // The input FieldInfo[] contains all fields numbered from 0 to infos.length - 1, and they are
+ // sorted, use it directly. This is an optimization when reading a segment with all fields
+ // since the FieldInfo[] is sorted.
+ byNumber = infos;
+ values = Arrays.asList(byNumber);
+ } else {
+ byNumber = new FieldInfo[maxFieldNumber + 1];
+ for (FieldInfo fieldInfo : infos) {
+ FieldInfo existing = byNumber[fieldInfo.number];
+ if (existing != null) {
+ throw new IllegalArgumentException(
+ "duplicate field numbers: "
+ + existing.name
+ + " and "
+ + fieldInfo.name
+ + " have: "
+ + fieldInfo.number);
+ }
+ byNumber[fieldInfo.number] = fieldInfo;
+ }
+ if (maxFieldNumber == infos.length - 1) {
+ // No fields are missing, use byNumber.
+ values = Arrays.asList(byNumber);
+ } else {
+ // The below code is faster than Arrays.stream(byNumber).filter(Objects::nonNull).toList(),
+ // mainly when the input FieldInfo[] is sorted, when reading a segment.
Review Comment:
If fieldNumberStrictlyAscending, and we've given ourselves permission to accept the input array, we can merely wrap it in Arrays.asList and we're done.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant merged PR #13327:
URL: https://github.com/apache/lucene/pull/13327
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1594462331
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -156,15 +148,38 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- List<FieldInfo> valuesTemp = new ArrayList<>(infos.length);
- byNumber = new FieldInfo[size];
- for (int i = 0; i < size; i++) {
- byNumber[i] = byNumberTemp[i];
- if (byNumberTemp[i] != null) {
- valuesTemp.add(byNumberTemp[i]);
+ if (fieldNumberStrictlyAscending && maxFieldNumber == infos.length - 1) {
+ // The input FieldInfo[] contains all fields numbered from 0 to infos.length - 1 and they are
+ // sorted, use it directly. This is an optimization when reading a segment with all fields
+ // since the FieldInfo[] is sorted.
+ byNumber = infos; // We could copy the input array, but do we need to?
Review Comment:
This is fairly internal stuff labelled lucene.experimental; I say yes but document it in the javadoc. Obviously look at existing callers to double-check this is fine.
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -139,17 +148,38 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- FieldInfo[] sortedFieldInfos = ArrayUtil.copyOfSubArray(infos, 0, infos.length);
- Arrays.sort(sortedFieldInfos, (fi1, fi2) -> Integer.compare(fi1.number, fi2.number));
- int maxFieldNumber = infos.length == 0 ? -1 : sortedFieldInfos[infos.length - 1].number;
- // If there are many fields and the max field number is greater than twice the number
- // of fields, then a map structure is more compact to store the by-number mapping.
- byNumber =
- maxFieldNumber >= 2 * infos.length && maxFieldNumber >= 32
- ? new MapFieldInfoByNumber(infos)
- : new ArrayFieldInfoByNumber(infos, maxFieldNumber);
- // The iteration of FieldInfo is ordered by ascending field number.
- values = Collections.unmodifiableCollection(Arrays.asList(sortedFieldInfos));
+ if (fieldNumberStrictlyAscending && maxFieldNumber == infos.length - 1) {
+ // The input FieldInfo[] contains all fields numbered from 0 to infos.length - 1 and they are
+ // sorted, use it directly. This is an optimization when reading a segment with all fields
+ // since the FieldInfo[] is sorted.
+ byNumber = infos; // We could copy the input array, but do we need to?
+ values = Arrays.asList(byNumber);
+ } else {
+ byNumber = new FieldInfo[maxFieldNumber + 1];
+ for (FieldInfo fieldInfo : infos) {
+ FieldInfo previous = byNumber[fieldInfo.number];
+ if (previous != null) {
+ throw new IllegalArgumentException(
+ "duplicate field numbers: "
+ + previous.name
+ + " and "
+ + fieldInfo.name
+ + " have: "
+ + fieldInfo.number);
+ }
+ byNumber[fieldInfo.number] = fieldInfo;
+ }
+ if (maxFieldNumber == infos.length - 1) {
+ // No fields are missing, use byNumber.
+ values = Arrays.asList(byNumber);
+ } else {
+ // The below code is faster than Arrays.stream(byNumber).filter(Objects::nonNull).toList(),
+ // mainly when the input FieldInfo[] is sorted, when reading a segment.
+ FieldInfo[] sortedFieldInfos = ArrayUtil.copyOfSubArray(infos, 0, infos.length);
Review Comment:
It looks a bit curious to be calling a method with "sub array" for what is actually the whole array. Maybe a convenience method copy(infos) should be provided. Could be deferred of course.
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -139,17 +148,38 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- FieldInfo[] sortedFieldInfos = ArrayUtil.copyOfSubArray(infos, 0, infos.length);
- Arrays.sort(sortedFieldInfos, (fi1, fi2) -> Integer.compare(fi1.number, fi2.number));
- int maxFieldNumber = infos.length == 0 ? -1 : sortedFieldInfos[infos.length - 1].number;
- // If there are many fields and the max field number is greater than twice the number
- // of fields, then a map structure is more compact to store the by-number mapping.
- byNumber =
- maxFieldNumber >= 2 * infos.length && maxFieldNumber >= 32
- ? new MapFieldInfoByNumber(infos)
- : new ArrayFieldInfoByNumber(infos, maxFieldNumber);
- // The iteration of FieldInfo is ordered by ascending field number.
- values = Collections.unmodifiableCollection(Arrays.asList(sortedFieldInfos));
+ if (fieldNumberStrictlyAscending && maxFieldNumber == infos.length - 1) {
+ // The input FieldInfo[] contains all fields numbered from 0 to infos.length - 1 and they are
+ // sorted, use it directly. This is an optimization when reading a segment with all fields
+ // since the FieldInfo[] is sorted.
+ byNumber = infos; // We could copy the input array, but do we need to?
+ values = Arrays.asList(byNumber);
+ } else {
+ byNumber = new FieldInfo[maxFieldNumber + 1];
+ for (FieldInfo fieldInfo : infos) {
+ FieldInfo previous = byNumber[fieldInfo.number];
Review Comment:
this is non-obvious. At a glance, we are retrieving the very same fieldInfo, yet supposedly it's the previous. Oh, maybe you mean "existing" fieldInfo as opposed to a fieldInfo ordered prior to this one?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1597476337
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -139,17 +148,38 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- FieldInfo[] sortedFieldInfos = ArrayUtil.copyOfSubArray(infos, 0, infos.length);
- Arrays.sort(sortedFieldInfos, (fi1, fi2) -> Integer.compare(fi1.number, fi2.number));
- int maxFieldNumber = infos.length == 0 ? -1 : sortedFieldInfos[infos.length - 1].number;
- // If there are many fields and the max field number is greater than twice the number
- // of fields, then a map structure is more compact to store the by-number mapping.
- byNumber =
- maxFieldNumber >= 2 * infos.length && maxFieldNumber >= 32
- ? new MapFieldInfoByNumber(infos)
- : new ArrayFieldInfoByNumber(infos, maxFieldNumber);
- // The iteration of FieldInfo is ordered by ascending field number.
- values = Collections.unmodifiableCollection(Arrays.asList(sortedFieldInfos));
+ if (fieldNumberStrictlyAscending && maxFieldNumber == infos.length - 1) {
+ // The input FieldInfo[] contains all fields numbered from 0 to infos.length - 1 and they are
+ // sorted, use it directly. This is an optimization when reading a segment with all fields
+ // since the FieldInfo[] is sorted.
+ byNumber = infos; // We could copy the input array, but do we need to?
+ values = Arrays.asList(byNumber);
+ } else {
+ byNumber = new FieldInfo[maxFieldNumber + 1];
+ for (FieldInfo fieldInfo : infos) {
+ FieldInfo previous = byNumber[fieldInfo.number];
+ if (previous != null) {
+ throw new IllegalArgumentException(
+ "duplicate field numbers: "
+ + previous.name
+ + " and "
+ + fieldInfo.name
+ + " have: "
+ + fieldInfo.number);
+ }
+ byNumber[fieldInfo.number] = fieldInfo;
+ }
+ if (maxFieldNumber == infos.length - 1) {
+ // No fields are missing, use byNumber.
+ values = Arrays.asList(byNumber);
+ } else {
+ // The below code is faster than Arrays.stream(byNumber).filter(Objects::nonNull).toList(),
+ // mainly when the input FieldInfo[] is sorted, when reading a segment.
+ FieldInfo[] sortedFieldInfos = ArrayUtil.copyOfSubArray(infos, 0, infos.length);
Review Comment:
I agree. I created another [PR](https://github.com/apache/lucene/pull/13360) to add ArrayUtil.copyOf().
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1597498694
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -156,15 +151,38 @@ public FieldInfos(FieldInfo[] infos) {
this.softDeletesField = softDeletesField;
this.parentField = parentField;
- List<FieldInfo> valuesTemp = new ArrayList<>(infos.length);
- byNumber = new FieldInfo[size];
- for (int i = 0; i < size; i++) {
- byNumber[i] = byNumberTemp[i];
- if (byNumberTemp[i] != null) {
- valuesTemp.add(byNumberTemp[i]);
+ if (fieldNumberStrictlyAscending && maxFieldNumber == infos.length - 1) {
+ // The input FieldInfo[] contains all fields numbered from 0 to infos.length - 1, and they are
+ // sorted, use it directly. This is an optimization when reading a segment with all fields
+ // since the FieldInfo[] is sorted.
+ byNumber = infos;
+ values = Arrays.asList(byNumber);
+ } else {
+ byNumber = new FieldInfo[maxFieldNumber + 1];
+ for (FieldInfo fieldInfo : infos) {
+ FieldInfo existing = byNumber[fieldInfo.number];
+ if (existing != null) {
+ throw new IllegalArgumentException(
+ "duplicate field numbers: "
+ + existing.name
+ + " and "
+ + fieldInfo.name
+ + " have: "
+ + fieldInfo.number);
+ }
+ byNumber[fieldInfo.number] = fieldInfo;
+ }
+ if (maxFieldNumber == infos.length - 1) {
+ // No fields are missing, use byNumber.
+ values = Arrays.asList(byNumber);
+ } else {
+ // The below code is faster than Arrays.stream(byNumber).filter(Objects::nonNull).toList(),
+ // mainly when the input FieldInfo[] is sorted, when reading a segment.
Review Comment:
Here maxFieldNumber > infos.length - 1, so some fields are missing in byNumber (byNumber[fieldNumber] == null). We must not return those null FieldInfos in values.iterator().
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Reduce memory usage of field maps in FieldInfos and BlockTree TermsReader. [lucene]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #13327:
URL: https://github.com/apache/lucene/pull/13327#discussion_r1585064742
##########
lucene/core/src/java/org/apache/lucene/codecs/lucene90/blocktree/Lucene90BlockTreeTermsReader.java:
##########
@@ -113,7 +114,8 @@ public final class Lucene90BlockTreeTermsReader extends FieldsProducer {
// produce DocsEnum on demand
final PostingsReaderBase postingsReader;
- private final Map<String, FieldReader> fieldMap;
+ private final FieldInfos fieldInfos;
+ private final IntObjectHashMap<FieldReader> fieldMap;
Review Comment:
Actually there are many usages of Map<Integer, Object>. I could open some PRs when memory (and perf) matters after IntObjectHashMap is in.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org