You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/08/24 13:41:11 UTC
[GitHub] [lucene] madrob opened a new pull request, #11715: RamUsageEstimator properly checks for Long cache
madrob opened a new pull request, #11715:
URL: https://github.com/apache/lucene/pull/11715
Add RamUsageEstimator.sizeOf(Integer)
Improve estimation for both Long and Integer to not count VM Cache values.
--
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
[GitHub] [lucene] madrob commented on pull request #11715: RamUsageEstimator properly checks for Long cache
Posted by GitBox <gi...@apache.org>.
madrob commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1227247964
@uschindler my latest patch fixes the comment and adds Integer size. if you strongly disagree with adding the Integer size then we can roll that back too, but I think since we're already doing checks for Long it is ok to add other types here.
--
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
[GitHub] [lucene] madrob commented on pull request #11715: RamUsageEstimator properly checks for Long cache
Posted by GitBox <gi...@apache.org>.
madrob commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1225956235
I went looking for the previous change and found it in LUCENE-9117. Adding David for review request, since he was touching this last.
The JLS spec requires an Integer cache of at least `[-128, 127]`, but in practice the upper bound can be higher. The Long cache is not required to exist, in current JVMs it looks like it is also hard coded for the same range, but no possibility to grow. I don't want to rely on vendor implementation of a JVM for the size of their boxed primitive caches though.
Maybe an aggressive compiler will figure out how to remove the checks that I'm adding here as well and we'll be stuck with misleading behavior again? If so, the other option is to update the comment so we're not claiming to be VM-cache-aware.
--
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
[GitHub] [lucene] uschindler commented on pull request #11715: RamUsageEstimator properly checks for Long cache
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1226112858
I still would not change the code here. Better to overestimate than having all Longs or Ints suddenly size=0 (when AOT jumps in).
I'd just remove the wrong comment.
And about Solr: Solr should use shallowSizeOf(Long.class) to tell Cafeine cache the reals size. That works consistent.
--
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
[GitHub] [lucene] uschindler commented on pull request #11715: Add Integer awareness to RamUsageEstimator.sizeOf
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1234443307
Yes. Wasn't that 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
[GitHub] [lucene] uschindler commented on pull request #11715: Add Integer awareness to RamUsageEstimator.sizeOf
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1234458503
I think all is fine here. It is in change log of 9.4 and in 9x branch. So will be in release when 9.4 is branched away.
--
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
[GitHub] [lucene] dweiss commented on pull request #11715: RamUsageEstimator properly checks for Long cache
Posted by GitBox <gi...@apache.org>.
dweiss commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1226096136
Darn, I can't remember writing that code. :) LGTM though. With smarter and smarter compilers such memory estimation may become tricky like hell...
--
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
[GitHub] [lucene] madrob merged pull request #11715: Add Integer awareness to RamUsageEstimator.sizeOf
Posted by GitBox <gi...@apache.org>.
madrob merged PR #11715:
URL: https://github.com/apache/lucene/pull/11715
--
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
[GitHub] [lucene] ben-manes commented on pull request #11715: Add Integer awareness to RamUsageEstimator.sizeOf
Posted by GitBox <gi...@apache.org>.
ben-manes commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1228006073
> I don't recommend to use caffeine because it uses Unsafe....
@uschindler fyi Caffeine 3.x does not use Unsafe, which is what Solr uses. Unfortunatelly we did not have any good options prior to Java 9 (VarHandles), as atomic references would add memory bloat and atomic field updaters would use reflection. When Solr updated to Java 11 it was able to make the switch.
--
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
[GitHub] [lucene] msokolov commented on pull request #11715: Add Integer awareness to RamUsageEstimator.sizeOf
Posted by GitBox <gi...@apache.org>.
msokolov commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1234437820
is it worth backporting to 9.x?
--
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
[GitHub] [lucene] uschindler commented on pull request #11715: RamUsageEstimator properly checks for Long cache
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1225972120
aaaaah... AOT, so my brain correlated that correctly to OpenJ9 (which also uses AOT)!
Yes, AOT could remove the check and then suddenly all Longs and Integers are zero size. At least your solution will not blow up when AOT jumps in. It will find identity in the obejcts and says "all longs/ints are cached", so size 0. The old code that Dawid removed went into an endless loop.
In my personal opinion, I would not check for the cache at all, unless we really overestimate for certain cases like and `Integer[]` array with thousands of small integers. But In Lucene code we have have this.
What is the real problem you are trying to solve?
Uwe
--
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
[GitHub] [lucene] madrob commented on pull request #11715: RamUsageEstimator properly checks for Long cache
Posted by GitBox <gi...@apache.org>.
madrob commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1225979052
We saw some weird behavior in Solr with a size-limited Caffeine cache not getting as many entries as we thought it should have. That turned out to be because sizeOf(Integer) was returning 1024 (Solr's default guess for how big something is), and while investigating that missing piece, I found the incongruity between the comments and the code around LongCache.
--
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
[GitHub] [lucene] uschindler commented on pull request #11715: RamUsageEstimator properly checks for Long cache
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1225923616
Impl looks ok. In early versions we had an auto detector for the size of the cache, but we disabled it due to a bug in OpenJ9 (not sure about it anymore).
The comments still suggest that the auto detector is still there.
I have mixed feelings if we really need the size so exact? Do we really have many Integers and Longs in arrays or maps/sets?
--
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
[GitHub] [lucene] madrob commented on pull request #11715: Add Integer awareness to RamUsageEstimator.sizeOf
Posted by GitBox <gi...@apache.org>.
madrob commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1234453507
@msokolov should be 090b05033e742c4db779dc3e0def83e8425b7ce3?
--
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
[GitHub] [lucene] uschindler commented on pull request #11715: RamUsageEstimator properly checks for Long cache
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1225990121
> We saw some weird behavior in Solr with a size-limited Caffeine cache not getting as many entries as we thought it should have. That turned out to be because sizeOf(Integer) was returning 1024 (Solr's default guess for how big something is), and while investigating that missing piece, I found the incongruity between the comments and the code around LongCache.
PRO TIP: On Guava's caches, I generally use the `weighter()` method in the cache builder to help the cache with a function how to calculate the "weight" of objects. This helps the cache more than exact sizes of elements. It allows it to figure out relatively large elements. Not sure if Caffeine has the same (I don't recommend to use caffeine because it uses Unsafe....).
--
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
[GitHub] [lucene] uschindler commented on pull request #11715: Add Integer awareness to RamUsageEstimator.sizeOf
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1228162195
Thanks sorry, I just remembered the Solr 8.x times and the discussions and also bugs about caffeine which affected testing.
--
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
[GitHub] [lucene] uschindler commented on pull request #11715: Add Integer awareness to RamUsageEstimator.sizeOf
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1234456942
Yes. It should be cherry picked for 9x branch
--
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
[GitHub] [lucene] msokolov commented on pull request #11715: Add Integer awareness to RamUsageEstimator.sizeOf
Posted by GitBox <gi...@apache.org>.
msokolov commented on PR #11715:
URL: https://github.com/apache/lucene/pull/11715#issuecomment-1234489241
great, thanks!
--
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