Fix Parquet metadata heap size accounting #8898

alamb · 2025-11-20T21:35:15Z

Which issue does this PR close?

We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.

Closes Parquet metadata size accounting overcounts Arc<T> #8897

Rationale for this change

While testing the upgrade to arrow 57.1.0 in DataFusion

Update to arrow, parquet to 57.1.0 datafusion#18820

I noticed that the reported size of the memory used by metadata grew by almost 33%

What changes are included in this PR?

Fix accounting for Arc, remove some other uneeded code

Are these changes tested?

Yes covered by existing tests (and changed)

Are there any user-facing changes?

More accurate memory accounting

etseidl · 2025-11-20T21:45:58Z

parquet/src/file/metadata/memory.rs

-            + self.as_ref().heap_size()
+        // Do not count the size of the Arc as that is accounted for by the
+        // caller (the object that contains the Arc).
+        self.as_ref().heap_size()


With this change:

let v = vec![0i32; 20]; println!("filled vec heap size {}", v.heap_size()); println!("size of vec {}", std::mem::size_of::<Vec<i32>>()); let av = Arc::new(v); println!("arc<vec> heap size {}", av.heap_size());

filled vec heap size 80 size of vec 24 arc<vec> heap size 80

The Vec is now on the heap, but that is not accounted for.

yeah, sorry, I pushed the correct fix for that

alamb · 2025-11-20T21:35:55Z

parquet/src/encryption/decrypt.rs

 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()


drive by cleanup -- this was added in #8671 and the new Arc::clone doesn't seem to be needed.

This was needed to be able to cast to Arc<dyn HeapSize> in order to use the impl HeapSize for Arc<dyn HeapSize>. Otherwise the other implementation (impl<T: HeapSize> HeapSize for Arc<T>) isn't used and the code with your change just derefs the contents of the Arc and calls its heap_size method, and the heap size of the Arc (ie. the size of its contents plus its reference counts) is silently skipped.

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:

arrow-rs/parquet/src/file/metadata/mod.rs

Line 296 in 123e6cd

std::mem::size_of::<Self>()

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>())

I think the issue is dyn HeapSize doesn't implement HeapSize, but even if it did the existing HeapSize impl for Arc wouldn't work because you can't use std::mem::size_of::<T>() on an unsized dyn object, and need to use std::mem::size_of_val.

So I am still not sure about this

I guess what I am confused about is why we have a dyn Heapsize in the first place -- we know the exact type of the decryptor: https://github.com/apache/arrow-rs/blob/main/parquet/src/encryption/decrypt.rs#L563

The decryptor referenced here is actually an Arc<dyn BlockDecryptor>:

arrow-rs/parquet/src/encryption/decrypt.rs

Line 565 in ed9efe7

footer_decryptor: Arc<dyn BlockDecryptor>,

We do only have one implementation of that trait at the moment though, but the original plan was to also support an AES-GCM-CTR decryptor (#7258)

alamb · 2025-11-20T21:45:23Z

parquet/src/file/metadata/mod.rs


        #[cfg(not(feature = "encryption"))]
-        let base_expected_size = 2766;
+        let base_expected_size = 2670;


it is somewhat smaller, but still pretty big

adamreeve · 2025-11-20T22:01:30Z

parquet/src/file/metadata/memory.rs

-            + 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()


The strong and weak counts are stored on the heap so aren't included in std::mem::size_of::<T>().

etseidl · 2025-11-20T22:01:38Z

parquet/src/file/metadata/memory.rs

-            + 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()


from Box::new

let x: Box<_> = Box::new(ArcInner { strong: atomic::AtomicUsize::new(1), weak: atomic::AtomicUsize::new(1), data, });

strong and weak are also on the heap, along with the T data.

size_of::<Arc<_>> returns 8, which is what I believe is accounted for in the size of the containers.

alamb

Thank you @adamreeve and @etseidl for humoring my false alarm.

I re-reviewed the code from stdlib and I agree what is currently on main is an accurate representation

alamb · 2025-11-20T22:12:51Z

parquet/src/encryption/decrypt.rs

 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()


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:

arrow-rs/parquet/src/file/metadata/mod.rs

Line 296 in 123e6cd

std::mem::size_of::<Self>()

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>())

alamb · 2025-11-20T22:15:28Z

parquet/src/encryption/decrypt.rs

 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()


So I am still not sure about this

alamb · 2025-11-20T22:17:02Z

parquet/src/file/metadata/memory.rs

-            + 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()


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

adamreeve · 2025-11-20T22:35:55Z

Thank you @adamreeve and @etseidl for humoring my false alarm.

I re-reviewed the code from stdlib and I agree what is currently on main is an accurate representation

No problem 😄. It would be nice to have an automated way to verify this and catch if any changes cause the memory usage to become inaccurate. I did manual testing with heaptrack to verify my changes but I'm not sure how easy it would be to automate that process.

alamb · 2025-11-21T11:33:41Z

No problem 😄. It would be nice to have an automated way to verify this and catch if any changes cause the memory usage to become inaccurate. I did manual testing with heaptrack to verify my changes but I'm not sure how easy it would be to automate that process.

I 100 agree with this. We maybe could shim the system allocator or something and count allocations 🤔 But as you say that is likely to be pretty brittle

adamreeve · 2025-11-26T03:07:53Z

I've opened a draft PR that overrides the global allocator to track allocations and this seems to be working well (#8925). I don't have a lot of time at the moment to keep working on this but will try to add some more test cases including encrypted files soon.

Fix Parquet metadata heap size accounting

c48de3d

github-actions bot added the parquet Changes to the parquet crate label Nov 20, 2025

Adjust

123e6cd

etseidl reviewed Nov 20, 2025

View reviewed changes

alamb commented Nov 20, 2025

View reviewed changes

adamreeve reviewed Nov 20, 2025

View reviewed changes

etseidl reviewed Nov 20, 2025

View reviewed changes

alamb commented Nov 20, 2025

View reviewed changes

alamb closed this Nov 20, 2025

This was referenced Nov 20, 2025

[Parquet] Account for FileDecryptor in ParquetMetaData heap size calculation #8671

Merged

Parquet metadata size accounting overcounts Arc<T> #8897

Closed

adamreeve mentioned this pull request Nov 26, 2025

[Parquet] Add metadata heap size test that tracks allocations #8924

Open

Fix Parquet metadata heap size accounting #8898

Fix Parquet metadata heap size accounting #8898

Uh oh!

Conversation

alamb commented Nov 20, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamreeve Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamreeve commented Nov 20, 2025

Uh oh!

alamb commented Nov 21, 2025

Uh oh!

adamreeve commented Nov 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

adamreeve Nov 20, 2025 •

edited

Loading