Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std: fix memory leak in ArrayHashMap #13001

Merged
merged 2 commits into from Oct 4, 2022
Merged

Conversation

GethDW
Copy link
Contributor

@GethDW GethDW commented Sep 28, 2022

No description provided.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Instead let's swap the order so that the ensureCapacity happens first, and then no cleanup code is needed.

@@ -773,9 +773,9 @@ pub fn ArrayHashMapUnmanaged(
}
}

try self.entries.ensureTotalCapacity(allocator, new_capacity);
const new_bit_index = try IndexHeader.findBitIndex(new_capacity);
const new_header = try IndexHeader.alloc(allocator, new_bit_index);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this could be a single memory allocation combining the index header and the array of entries

Copy link
Member

Choose a reason for hiding this comment

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

The fields for ArrayHashMap are:

        /// It is permitted to access this field directly.
        entries: DataList = .{},

        /// When entries length is less than `linear_scan_max`, this remains `null`.
        /// Once entries length grows big enough, this field is allocated. There is
        /// an IndexHeader followed by an array of Index(I) structs, where I is defined
        /// by how many total indexes there are.
        index_header: ?*IndexHeader = null,

Having the header be a separate allocation is an optimization for maps that are below a certain size threshold.

You are welcome to experiment with changing the memory layout here - if you can demonstrate a perf enhancement without breaking the API, that's an easy merge.

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Thanks!

@andrewrk
Copy link
Member

I'd like to add a test to this before merging, especially since there seems to be some interest in reworking the implementation. Should be pretty straightforward with std.testing.checkAllAllocationFailures.

@andrewrk andrewrk merged commit 9d5462d into ziglang:master Oct 4, 2022
@andrewrk
Copy link
Member

andrewrk commented Oct 4, 2022

Thanks!

@GethDW GethDW deleted the array-hash-fix branch October 4, 2022 08:23
Vexu pushed a commit to Vexu/zig that referenced this pull request Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants