Migrate HashingLRUCache to Abseil containers [chromium/src : main]
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
I want to switch to absl::flat_hash_map for the LRU cache in net::AddressSorterPosix to reduce the overhead on misses, but it's more efficient in terms of binary size to switch everything at once.
It doesn't appear to be possible to avoid the increase in source volume, but I limited it by splitting the lru_cache.h header into three parts.
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job android-pixel10-perf/rendering.mobile complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16e0d4eac90000
Related details
- Adam Rice
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
// key type.How many things do we need to update to teach them about AbslHashValue?
I think this is probably OK as an intermediate step, but in the final state, it seems like it'd be better to not have this shim and fallback.
namespace internal {As these bits are new, can we avoid using `base::internal` directly per the style guide guidance? https://google.github.io/styleguide/cppguide.html#Namespace_Names:~:text=Use%20namespaces%20with%20%22internal%22%20in%20the%20name%20to%20document%20parts%20of%20an%20API%20that%20should%20not%20be%20mentioned%20by%20users%20of%20the%20API.
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 2 comments![]( "Open in Gerrit")
// key type.How many things do we need to update to teach them about AbslHashValue?
I think this is probably OK as an intermediate step, but in the final state, it seems like it'd be better to not have this shim and fallback.
Because Abseil's hash can fall back to std::hash, in practice nothing needed updating. New users of HashingLRUCache are likely to see this error if their key type is not yet hashable.
namespace internal {As these bits are new, can we avoid using `base::internal` directly per the style guide guidance? https://google.github.io/styleguide/cppguide.html#Namespace_Names:~:text=Use%20namespaces%20with%20%22internal%22%20in%20the%20name%20to%20document%20parts%20of%20an%20API%20that%20should%20not%20be%20mentioned%20by%20users%20of%20the%20API.
I don't understand what you are asking for. HashingLRUCacheKeyIndex, AbslHasher and AbslEqual are all parts of the API that shouldn't be mentioned outside the implementation of HashingLRUCache, which is exactly when the style guide says to use "internal". Do you mean we should call it "internal_hashing_lru_cache" instead?
Related details
- Daniel Cheng
Daniel Cheng (Gerrit)
Daniel Cheng added 2 comments![]( "Open in Gerrit")
// key type.Adam RiceHow many things do we need to update to teach them about AbslHashValue?
I think this is probably OK as an intermediate step, but in the final state, it seems like it'd be better to not have this shim and fallback.
Because Abseil's hash can fall back to std::hash, in practice nothing needed updating. New users of HashingLRUCache are likely to see this error if their key type is not yet hashable.
I guess I'm wondering why we have this layer of indirection; why can't we just use DefaultHashContainerHash and DefaultHashContainerEq directly?
namespace internal {Adam RiceAs these bits are new, can we avoid using `base::internal` directly per the style guide guidance? https://google.github.io/styleguide/cppguide.html#Namespace_Names:~:text=Use%20namespaces%20with%20%22internal%22%20in%20the%20name%20to%20document%20parts%20of%20an%20API%20that%20should%20not%20be%20mentioned%20by%20users%20of%20the%20API.
I don't understand what you are asking for. HashingLRUCacheKeyIndex, AbslHasher and AbslEqual are all parts of the API that shouldn't be mentioned outside the implementation of HashingLRUCache, which is exactly when the style guide says to use "internal". Do you mean we should call it "internal_hashing_lru_cache" instead?
Do you mean we should call it "internal_hashing_lru_cache" instead?
Yes (though the examples in the style give `_internal` as a suffix instead of a prefix)
Related details
- Adam Rice
Adam Rice (Gerrit)
Adam Rice added 2 comments![]( "Open in Gerrit")
Adam RiceHow many things do we need to update to teach them about AbslHashValue?
I think this is probably OK as an intermediate step, but in the final state, it seems like it'd be better to not have this shim and fallback.
Daniel ChengBecause Abseil's hash can fall back to std::hash, in practice nothing needed updating. New users of HashingLRUCache are likely to see this error if their key type is not yet hashable.
I guess I'm wondering why we have this layer of indirection; why can't we just use DefaultHashContainerHash and DefaultHashContainerEq directly?
It turns out we can, so I switched to that. Previously I was using a hacky method to find out what hash function Abseil would use, and it didn't always work. So I added the indirection to work around it, When I switched to the current method I didn't realise I could remove the indirection.
Fixed.
Adam RiceAs these bits are new, can we avoid using `base::internal` directly per the style guide guidance? https://google.github.io/styleguide/cppguide.html#Namespace_Names:~:text=Use%20namespaces%20with%20%22internal%22%20in%20the%20name%20to%20document%20parts%20of%20an%20API%20that%20should%20not%20be%20mentioned%20by%20users%20of%20the%20API.
Daniel ChengI don't understand what you are asking for. HashingLRUCacheKeyIndex, AbslHasher and AbslEqual are all parts of the API that shouldn't be mentioned outside the implementation of HashingLRUCache, which is exactly when the style guide says to use "internal". Do you mean we should call it "internal_hashing_lru_cache" instead?
Do you mean we should call it "internal_hashing_lru_cache" instead?
Yes (though the examples in the style give `_internal` as a suffix instead of a prefix)
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |