Skip to content

refactor: centralizing shared-allocation accounting for Arc DFHeapSize impls#23349

Open
saadtajwar wants to merge 2 commits into
apache:mainfrom
saadtajwar:saadt/centralize-arc-dfheapsize-impls
Open

refactor: centralizing shared-allocation accounting for Arc DFHeapSize impls#23349
saadtajwar wants to merge 2 commits into
apache:mainfrom
saadtajwar:saadt/centralize-arc-dfheapsize-impls

Conversation

@saadtajwar

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

DFHeapSize depends on a critical invariant: shared heap allocations must be counted once per traversal context. If Arc implementations drift, memory accounting becomes inconsistent and hard to reason about.

Consolidating the one-time-accounting logic improves:

Correctness durability (single source of truth for Arc dedup behavior)
Maintainability (less repeated logic)
Reviewability (future changes touch one helper)

What changes are included in this PR?

Helper functions for Arc allocation identity and deduplication, namely for providing pointer extraction & DFHeapSizeCtx set membership checks, with the DFHeapSize implementations on Arc backed types using the new helpers

Are these changes tested?

Yes, the below are passing:

cargo test -p datafusion-common heap_size --lib
cargo test -p datafusion-common --doc heap_size

Are there any user-facing changes?

No

@saadtajwar

Copy link
Copy Markdown
Contributor Author

cc @kosiew - thanks in advance for any feedback! :)

@github-actions github-actions Bot added the common Related to common crate label Jul 6, 2026

@mkleen mkleen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saadtajwar Thanks for working on this! I left a comment.

}
}

fn count_allocation_once(ptr: usize, ctx: &mut DFHeapSizeCtx) -> bool {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add this function directly to the DFHeapSizeCtx:

impl DFHeapSizeCtx {
    fn count_allocation_once(mut self, ptr: usize) -> bool {
        self.seen.insert(ptr)
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to common crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Centralize shared-allocation accounting for Arc DFHeapSize implementations

2 participants