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

Remove extraneous memory allocations in C #437

Merged
merged 1 commit into from Mar 14, 2023
Merged

Remove extraneous memory allocations in C #437

merged 1 commit into from Mar 14, 2023

Conversation

ianks
Copy link
Contributor

@ianks ianks commented Mar 14, 2023

This PR makes use of rb_str_buf_new, so we can read directly into it. This avoids an an extraneous ALLOC_N and corresponding xfree for every item that is read.

@ianks ianks requested a review from casperisfine March 14, 2023 02:26
Comment on lines 715 to 716
input_data = rb_str_buf_new(cached_key.size);
if (bs_read_contents(current_fd, current_key.size, input_data, &errno_provenance) < 0) goto fail_errno;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the return value of bs_read_contents is always checked against < 0, you could move rb_str_buf_new inside, and return Qfalse on failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, made those adjustments in d1c01f6

Copy link
Contributor

@casperisfine casperisfine left a comment

Choose a reason for hiding this comment

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

LGTM. Please squash you commits though.

@ianks
Copy link
Contributor Author

ianks commented Mar 14, 2023

Should I handle changelog / merging?

@casperisfine casperisfine merged commit 7639d0d into main Mar 14, 2023
@casperisfine
Copy link
Contributor

Thanks.

No need for a changelog entry, this is a minor optimization without any behavior change.

@shopify-shipit shopify-shipit bot temporarily deployed to rubygems March 14, 2023 14:06 Inactive
@ianks ianks deleted the less-alloc branch March 14, 2023 15:47
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

2 participants