alamb commented on code in PR #8898:
URL: https://github.com/apache/arrow-rs/pull/8898#discussion_r2547850056
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -582,7 +582,7 @@ impl PartialEq for FileDecryptor {
impl HeapSize for FileDecryptor {
fn heap_size(&self) -> usize {
self.decryption_properties.heap_size()
- + (Arc::clone(&self.footer_decryptor) as Arc<dyn
HeapSize>).heap_size()
Review Comment:
I think what is supposed to happen is:
The size of `Arc` itself (e.g. the pointer and ref count) is accounted for
in the size of the `FileDecryptor` struct. In this case, it is a field on
`ParquetMetadata` and this is accounted for here:
https://github.com/apache/arrow-rs/blob/123e6cd402db9c4cc121581d2a2a4aa273be5c23/parquet/src/file/metadata/mod.rs#L296
The size of the `FooterDecryptor` (what is pointed to by the Arc) is
accounted for by the `impl<T: HeapSize> HeapSize for Arc<T> {` -- specifically
the `std::mem::size_of::<T>()`)
##########
parquet/src/encryption/decrypt.rs:
##########
@@ -582,7 +582,7 @@ impl PartialEq for FileDecryptor {
impl HeapSize for FileDecryptor {
fn heap_size(&self) -> usize {
self.decryption_properties.heap_size()
- + (Arc::clone(&self.footer_decryptor) as Arc<dyn
HeapSize>).heap_size()
Review Comment:
So I am still not sure about this
##########
parquet/src/file/metadata/memory.rs:
##########
@@ -95,21 +95,16 @@ impl<K: HeapSize, V: HeapSize> HeapSize for HashMap<K, V> {
impl<T: HeapSize> HeapSize for Arc<T> {
fn heap_size(&self) -> usize {
- // Arc stores weak and strong counts on the heap alongside an instance
of T
- 2 * std::mem::size_of::<usize>() + std::mem::size_of::<T>() +
self.as_ref().heap_size()
- }
-}
-
-impl HeapSize for Arc<dyn HeapSize> {
- fn heap_size(&self) -> usize {
- 2 * std::mem::size_of::<usize>()
- + std::mem::size_of_val(self.as_ref())
- + self.as_ref().heap_size()
+ // Do not count the size of the Arc itself that is accounted for by the
+ // caller (the object that contains the Arc).
+ std::mem::size_of::<T>() + self.as_ref().heap_size()
Review Comment:
After reviewing the code, I agree with the original codes assement of this
structure's heap usage
The atomic counts are actually stored on the heap as well
https://doc.rust-lang.org/src/alloc/sync.rs.html#265
https://doc.rust-lang.org/src/alloc/sync.rs.html#371-378
--
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]
