msfroh commented on PR #16657: URL: https://github.com/apache/lucene/pull/16657#issuecomment-5625039695
@sgup432 -- I was comparing this to the 10.x branch logic. I see that there, we have the top-level `uniqueQueries`, which keeps track of the distinct queries in the cache. When a query is (potentially) added, we replace it with the canonical representation (i.e. the one already in `uniqueQueries`). That's why we're able to use `IdentityHashMap` at the leaf level, since we're guaranteed that if the query is in the cache, then we're using the canonical instance. Now that you've partitioned the cache at the top layer, I'm wondering if it makes sense to move the `uniqueQueries` map (and associated `mostRecentlyUsedQueries` view of the `keySet()`) into LRUQueryCachePartition. Then you can canonicalize the `query` before you create the `queryCacheKey` [here](https://github.com/apache/lucene/blob/372b1db88a5614ba3f0a927ab05e858e5087782e/lucene/core/src/java/org/apache/lucene/search/LRUQueryCache.java#L433). Even with your change as written, if two equal queries happen to land on two different segments, the associated `QueryCacheKey` objects will reference different instances of the same `Query` object, which wastes some memory. So replacing with the canonical instance of a `Query` has some benefit. -- 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]
