Introduce singleton for PackedInts.NullReader of size 256 #13232
Conversation
Size 256 is very common here throguh 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
| @@ -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); | |||
There was a problem hiding this 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.
There was a problem hiding this comment.
Renamed + made it return 0 now. Makes sense to me :)
There was a problem hiding this comment.
hmm this causes an annoying test failure .. let me see what I can do about it
There was a problem hiding this comment.
95fb423 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.
There was a problem hiding this comment.
Hmm agreed. Sorry for the suggestion. Let's not tweak ramBytesUsed then?
There was a problem hiding this comment.
No worries! ++ not worth the hassle :) -> Reverted the change
|
Thanks for taking a look Adrien!
Done :) Hope 9.11.0 was correct? |
Size 256 is very common here throguh 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
Correct estimate for singleton to return `0` and use custom accumulator in tests to fix assertions. Tried not doing this in apache#13232 but turns out we need a little complexity here since the singleton is recognized by the `RamUsageTester` in so far that is only counted once if it shows up repeatedly in an array or so. fixes apache#13249
…3250) Correct estimate for singleton to return `0` and use custom accumulator in tests to fix assertions. Tried not doing this in #13232 but turns out we need a little complexity here since the singleton is recognized by the `RamUsageTester` in so far that is only counted once if it shows up repeatedly in an array or so. fixes #13249
…3250) Correct estimate for singleton to return `0` and use custom accumulator in tests to fix assertions. Tried not doing this in #13232 but turns out we need a little complexity here since the singleton is recognized by the `RamUsageTester` in so far that is only counted once if it shows up repeatedly in an array or so. fixes #13249
Size 256 is very common here through the monotonic long values default page size. In ES we're seeing many MB O(10M) of duplicate instances of this size relatively quickly.
=> adding a singleton for it to save some heap