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

Improved memory management code in ParseSoaReply() #23628

Merged
merged 1 commit into from
Oct 23, 2018

Conversation

uttampawar
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
    Test results:
    $ python tools/test.py -I 'parallel/test-dns*'
    [00:00|% 100|+ 12|- 0]: Done

$ make test
[06:23|% 100|+ 2429|- 2]: Done
Failed tests are not due to this change.

This change was to improve memory management by use of MallocedBuffer instead of malloc()/free() calls. I further removed use of structure "ares_soa_reply" by using just local variables which removed 2 additional malloc/free calls via ares_expand_name().

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. labels Oct 12, 2018
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@addaleax addaleax added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 12, 2018
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with a comment.

@@ -1064,30 +1064,29 @@ int ParseSoaReply(Environment* env,
/* Can't use ares_parse_soa_reply() here which can only parse single record */
unsigned int ancount = cares_get_16bit(buf + 6);
unsigned char* ptr = buf + NS_HFIXEDSZ;
char* name;
char* rr_name;
int rr_type, rr_len;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this line be here?

Copy link
Member

Choose a reason for hiding this comment

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

It does seem to be added by accident?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjihrig @BridgeAR Do you mean line# 1067?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. It looks related to merge conflicts or something (I'm pretty sure I removed that line recently).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjihrig Yes you are right, that line is not in the mainline. Is it okay if I update the PR with removal of that line (rr_len and rr_type)?

@Trott
Copy link
Member

Trott commented Oct 14, 2018

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Need a better abstration

@@ -1064,30 +1064,28 @@ int ParseSoaReply(Environment* env,
/* Can't use ares_parse_soa_reply() here which can only parse single record */
unsigned int ancount = cares_get_16bit(buf + 6);
unsigned char* ptr = buf + NS_HFIXEDSZ;
char* name;
char* rr_name;
MallocedBuffer<char> name;
Copy link
Contributor

@refack refack Oct 18, 2018

Choose a reason for hiding this comment

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

IMHO you are using the wrong abstraction.
See the following guidelines:
ES.1: Prefer the standard library to other libraries and to “handcrafted code”
ES.2: Prefer suitable abstractions to direct use of language features

I think the better utility here is unique_ptr, since you don't track the size in conjunction with the allocated memory.
Also according to the docs, the buffer need to be freed with ares_free_string
so:

Suggested change
MallocedBuffer<char> name;
struct AresDeleter {
void operator()(char* ptr) const { ares_free_string(ptr); } noexcept
};
using ares_unique_ptr = std::unique_ptr<char, AresDeleter>;
char* name_tmp;
long temp_len; // NOLINT(runtime/int)
int status = ares_expand_name(ptr, buf, len, &name_tmp, &temp_len);
ares_unique_ptr name(name_tmp);
...
// In the smallest scope.
char* rr_name_tmp;
// Don't reuse status
int status2 = ares_expand_name(ptr, buf, len, &rr_name_tmp, &temp_len);
ares_unique_ptr rr_name(rr_name_tmp);

Notice rr_name should be in the smallest necessary scope:
ES.5: Keep scopes small

Copy link
Contributor

@refack refack Oct 18, 2018

Choose a reason for hiding this comment

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

Since there is no further transfer of ownership, I think that if you use this pattern, i.e.:

long temp_len;  // NOLINT(runtime/int)
char* name_tmp;
int status = ares_expand_name(ptr, buf, len, &name_tmp, &temp_len);
const ares_unique_ptr name(name_tmp);

You can use a const ares_unique_ptr getting more explicit code, and more compiler validation.
From CppReference:

Only non-const unique_ptr can transfer the ownership of the managed object to another unique_ptr. If an object's lifetime is managed by a const std::unique_ptr, it is limited to the scope in which the pointer was created.

return ARES_EBADRESP;
}
ptr += temp_len + NS_QFIXEDSZ;

for (unsigned int i = 0; i < ancount; i++) {
status = ares_expand_name(ptr, buf, len, &rr_name, &temp_len);
status = ares_expand_name(ptr, buf, len, &rr_name.data, &temp_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

ES.26: Don’t use a variable for two unrelated purposes

@@ -1098,76 +1096,67 @@ int ParseSoaReply(Environment* env,

/* only need SOA */
if (rr_type == ns_t_soa) {
ares_soa_reply soa;
MallocedBuffer<char> nsname;
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto


status = ares_expand_name(ptr, buf, len, &soa.nsname, &temp_len);
status = ares_expand_name(ptr, buf, len, &nsname.data, &temp_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

"

break;
}
ptr += temp_len;

status = ares_expand_name(ptr, buf, len, &soa.hostmaster, &temp_len);
status = ares_expand_name(ptr, buf, len, &hostmaster.data, &temp_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

"

@refack refack self-assigned this Oct 18, 2018
@refack
Copy link
Contributor

refack commented Oct 18, 2018

Hello @uttampawar and thank you for the contribution.
I've left some comments on how to get the code to conform to our guidelines. I'm happy to help with anything you need.

@uttampawar
Copy link
Contributor Author

@refack Thanks for the feedback. I did read up on C++ core guidelines as you suggested and it make sense to use unique_ptr interface for small usages like above. Let me come back with suggestion/modification.

@uttampawar
Copy link
Contributor Author

@addaleax @refack I like the solution suggested by @refack to use unique_ptr. At the same time, I could be wrong but I believe MallocedBuffer was introduced to create a common interface for all memory management patterns especially malloc() and free(). Also, IMHO in this case MallocedBuffer destructor should be using cares API (ares_free_string()) to free the memory instead of free(). Thanks to @refack for pointing that out.
IMHO, suggested solution is okay to use just in one place, but could it cause code bloat if it has to be repeated at multiple places? Looking for additional feedback.

@refack
Copy link
Contributor

refack commented Oct 18, 2018

@uttampawar thank you for following up!

I believe MallocedBuffer was introduced to create a common interface for all memory management patterns especially malloc() and free().

IMHO MallocedBuffer has many uses, and there are cases where it fits 100%, but I don't think it should be used for all memory management.
I think this case a unique_ptr specialization fits better.

As for code bloat, you could define:

struct AresDeleter {
  void operator()(char* ptr) const { ares_free_string(ptr); } noexcept
};
using ares_unique_ptr = std::unique_ptr<char, AresDeleter>;

in cares_wrap.h. That way all cares related memory management and ownership taking code could reuse it.

@addaleax
Copy link
Member

(Fwiw, in this case it would be std::unique_ptr<char[]>, not just char – we’ve apparently had trouble with shared_ptr on this point in the past, but for unique_ptr it should work.)

@uttampawar
Copy link
Contributor Author

@addaleax @refack It looks like we have an agreement on using unique_ptr instead of MallocedBuffer API for this case. I agree with suggestion of putting 'AresDeleter' in appropriate header file. I'll update the PR.

@uttampawar uttampawar force-pushed the malloced-buffer-cares branch from d5800ce to 16ca770 Compare October 19, 2018 22:40
@uttampawar
Copy link
Contributor Author

@refack @addaleax I've updated my PR with new changes as discussed, using std::unique_ptr instead of MallocedBuffer<>.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

ares_unique_ptr looks good.
Left comments about unnecessary var recycling.

return ARES_EBADRESP;
}
ptr += temp_len + NS_QFIXEDSZ;

char* rr_name_temp;
Copy link
Contributor

Choose a reason for hiding this comment

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

could you move this inside the for? ES.5: Keep scopes small
also please declare a new long temp_rr_len and int status2 ES.26: Don’t use a variable for two unrelated purposes

Reusing variables has no runtime benefit, and has the risk of misuse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about that but wasn't sure. Thanks for clarifying.


status = ares_expand_name(ptr, buf, len, &soa.nsname, &temp_len);
status = ares_expand_name(ptr, buf, len, &nsname_temp, &temp_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

@uttampawar uttampawar Oct 22, 2018

Choose a reason for hiding this comment

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

@refack Currently the status to return is captured in one variable 'status'. In a for loop, all the 'break' statements breaks out of loop and return some status. Having multiple of these small scoped variables breaking the flow by having multiple return statements instead of break stmt. I've not seen any explicit reference, but is this acceptable coding standard?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO what you did is great, that's exactly one of the reasons to avoid recycling.
Now you stop the function on first error and propagate it, and not continue to process in an error state.
Double win 🥇

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW: it's was simpler to do this now because of the RAII semantics of ares_unique_ptr.
Triple win 🏆

@refack
Copy link
Contributor

refack commented Oct 20, 2018

I've updated my PR with new changes as discussed, using std::unique_ptr instead of MallocedBuffer<>.

Thanks, IMHO that part looks nice and succinct 👍

@addaleax
Copy link
Member

@uttampawar uttampawar force-pushed the malloced-buffer-cares branch from 16ca770 to c5a4017 Compare October 23, 2018 00:37
@uttampawar
Copy link
Contributor Author

uttampawar commented Oct 23, 2018

@refack @addaleax I've rebased this patch on current master, and made changes addressing all the concerns. Looking for any feedback you may have.
Some of the test results are,

$ make -j4 test
...
[==========] 80 tests from 10 test cases ran. (194 ms total)
[ PASSED ] 80 tests.
...
[01:58|% 100|+ 2442|- 0]: Done

$ git log $TRAVIS_COMMIT_RANGE --pretty=format:'%h' --no-merges | tail -1 | xargs npx core-validate-commit --no-validate-metadata
npx: installed 13 in 2.01s
✔ 02aced6f21fbef6949a3a9ebdeb20454c4a99909
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
⚠ 0:50 Title should be <= 50 columns. title-length

@addaleax
Copy link
Member

@refack Could you take another look?

@refack
Copy link
Contributor

refack commented Oct 23, 2018

@refack refack removed their assignment Oct 23, 2018
@refack refack merged commit acb363f into nodejs:master Oct 23, 2018
@Trott
Copy link
Member

Trott commented Oct 23, 2018

Thanks for the contribution! 🎉

(If you're interested in other possible contributions to Node.js but don't have a good idea of where to start looking, some ideas are posted at https://www.nodetodo.org/next-steps/.)

@uttampawar
Copy link
Contributor Author

@Trott @refack Thank you for your support. I'll definitely look into above site for more ideas but I'm more interested in performance related opportunities specially where we can leverage CPU or platform features.

@uttampawar uttampawar deleted the malloced-buffer-cares branch October 24, 2018 02:56
targos pushed a commit that referenced this pull request Oct 24, 2018
Introduced use of smart pointers instead of MallocedBuffer to manage
memory allocated in the cares library.

PR-URL: #23628
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins
Copy link
Contributor

Should this be backported to v8.x and v10.x or should it bake a bit more

codebytere pushed a commit that referenced this pull request Dec 13, 2018
Introduced use of smart pointers instead of MallocedBuffer to manage
memory allocated in the cares library.

PR-URL: #23628
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
Introduced use of smart pointers instead of MallocedBuffer to manage
memory allocated in the cares library.

PR-URL: #23628
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@codebytere codebytere mentioned this pull request Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. cares Issues and PRs related to the c-ares dependency or the cares_wrap binding. code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants