You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "original-brownbear (via GitHub)" <gi...@apache.org> on 2024/03/28 13:18:32 UTC
[PR] Introduce singleton for PackedInts.NullReader of size 256 [lucene]
original-brownbear opened a new pull request, #13232:
URL: https://github.com/apache/lucene/pull/13232
Size 256 is very common here through the monotonic long values default page size. In ES we're seing many MB O(10M) of duplicate instances of this size relatively quickly.
=> adding a singleton for it to save some heap
--
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] Introduce singleton for PackedInts.NullReader of size 256 [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear commented on code in PR #13232:
URL: https://github.com/apache/lucene/pull/13232#discussion_r1543755288
##########
lucene/core/src/java/org/apache/lucene/util/packed/PackedInts.java:
##########
@@ -566,10 +566,20 @@ public String toString() {
/** A {@link Reader} which has all its values equal to 0 (bitsPerValue = 0). */
public static final class NullReader extends Reader {
+ private static final NullReader DEFAULT_MONOTONIC_LONG_VALUES_PAGE_SIZE =
+ new NullReader(MonotonicLongValues.DEFAULT_PAGE_SIZE);
Review Comment:
[95fb423](https://github.com/apache/lucene/pull/13232/commits/95fb423e182bc51bd0a3e6bb71f2ae759ee597ec) is pretty much the best I could come up with for the test, wdyt? I don't have a good intuition if the accuracy gain on the ram bytes method is worth the test noise I must admit.
--
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] Introduce singleton for PackedInts.NullReader of size 256 [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear commented on PR #13232:
URL: https://github.com/apache/lucene/pull/13232#issuecomment-2026126418
Thanks for taking a look Adrien!
> Can you add a CHANGES entry?
Done :) Hope 9.11.0 was correct?
--
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] Introduce singleton for PackedInts.NullReader of size 256 [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear commented on code in PR #13232:
URL: https://github.com/apache/lucene/pull/13232#discussion_r1543739530
##########
lucene/core/src/java/org/apache/lucene/util/packed/PackedInts.java:
##########
@@ -566,10 +566,20 @@ public String toString() {
/** A {@link Reader} which has all its values equal to 0 (bitsPerValue = 0). */
public static final class NullReader extends Reader {
+ private static final NullReader DEFAULT_MONOTONIC_LONG_VALUES_PAGE_SIZE =
+ new NullReader(MonotonicLongValues.DEFAULT_PAGE_SIZE);
Review Comment:
hmm this causes an annoying test failure .. let me see what I can do about it
--
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] Introduce singleton for PackedInts.NullReader of size 256 [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear commented on code in PR #13232:
URL: https://github.com/apache/lucene/pull/13232#discussion_r1544399427
##########
lucene/core/src/java/org/apache/lucene/util/packed/PackedInts.java:
##########
@@ -566,10 +566,20 @@ public String toString() {
/** A {@link Reader} which has all its values equal to 0 (bitsPerValue = 0). */
public static final class NullReader extends Reader {
+ private static final NullReader DEFAULT_MONOTONIC_LONG_VALUES_PAGE_SIZE =
+ new NullReader(MonotonicLongValues.DEFAULT_PAGE_SIZE);
Review Comment:
No worries! ++ not worth the hassle :) -> Reverted the change
--
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] Introduce singleton for PackedInts.NullReader of size 256 [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz merged PR #13232:
URL: https://github.com/apache/lucene/pull/13232
--
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] Introduce singleton for PackedInts.NullReader of size 256 [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13232:
URL: https://github.com/apache/lucene/pull/13232#discussion_r1543624600
##########
lucene/core/src/java/org/apache/lucene/util/packed/PackedInts.java:
##########
@@ -566,10 +566,20 @@ public String toString() {
/** A {@link Reader} which has all its values equal to 0 (bitsPerValue = 0). */
public static final class NullReader extends Reader {
+ private static final NullReader DEFAULT_MONOTONIC_LONG_VALUES_PAGE_SIZE =
+ new NullReader(MonotonicLongValues.DEFAULT_PAGE_SIZE);
Review Comment:
Can you use `PackedLongValues.DEFAULT_PAGE_SIZE` instead (the parent class)? (and rename the constant)
Separately I wonder if we should override `#ramBytesUsed()` to return 0 on this instance now that it's a singleton.
--
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] Introduce singleton for PackedInts.NullReader of size 256 [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear commented on code in PR #13232:
URL: https://github.com/apache/lucene/pull/13232#discussion_r1543715181
##########
lucene/core/src/java/org/apache/lucene/util/packed/PackedInts.java:
##########
@@ -566,10 +566,20 @@ public String toString() {
/** A {@link Reader} which has all its values equal to 0 (bitsPerValue = 0). */
public static final class NullReader extends Reader {
+ private static final NullReader DEFAULT_MONOTONIC_LONG_VALUES_PAGE_SIZE =
+ new NullReader(MonotonicLongValues.DEFAULT_PAGE_SIZE);
Review Comment:
Renamed + made it return `0` now. Makes sense to me :)
--
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] Introduce singleton for PackedInts.NullReader of size 256 [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13232:
URL: https://github.com/apache/lucene/pull/13232#discussion_r1544135442
##########
lucene/core/src/java/org/apache/lucene/util/packed/PackedInts.java:
##########
@@ -566,10 +566,20 @@ public String toString() {
/** A {@link Reader} which has all its values equal to 0 (bitsPerValue = 0). */
public static final class NullReader extends Reader {
+ private static final NullReader DEFAULT_MONOTONIC_LONG_VALUES_PAGE_SIZE =
+ new NullReader(MonotonicLongValues.DEFAULT_PAGE_SIZE);
Review Comment:
Hmm agreed. Sorry for the suggestion. Let's not tweak ramBytesUsed then?
--
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