Skip to content

netsocket: dns: make dns-cache-size:0 remove whole DNS cache code #7239

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

Merged

Conversation

TeroJaasko
Copy link
Contributor

Description

Setting ""nsapi.dns-cache-size": 0" still left some of the DNS
caching code in. Add crude #if to remove all of it.
This allows one to save 429 bytes of flash and 48 bytes of RAM on
ARMC5 builds.

Pull request type

[x] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@0xc0170 0xc0170 requested a review from a team June 18, 2018 12:34
@mikaleppanen
Copy link

This is modified also in: #7207

This pull request can be used in case application does not want to use the cache at all. That was not possible on previous lwip implementation (unless changing not exposed lwip internal configuration settings), but is good feature for new DNS.

SeppoTakalo
SeppoTakalo previously approved these changes Jun 18, 2018
Copy link
Contributor

@SeppoTakalo SeppoTakalo left a comment

Choose a reason for hiding this comment

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

Using a SingletonPointer would have prevented that. It does lazy construction, so if the object is not referenced, it's constructor would not be referenced either, allowing linker to drop it.

kjbracey
kjbracey previously approved these changes Jun 18, 2018
Copy link
Contributor

@kjbracey kjbracey left a comment

Choose a reason for hiding this comment

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

This will need to be rebased on that other PR #7207, but seems fine to me.

@kjbracey
Copy link
Contributor

I don't think SingletonPtr totally solves this case - it would eliminate the static cache structures not referenced due to the #if > 0 check, but probably generate an "unreferenced static" warning. So the combined PRs should have the SingletonPtrs surrounded by the #if > 0

mikaleppanen
mikaleppanen previously approved these changes Jun 19, 2018
@TeroJaasko
Copy link
Contributor Author

Actually the original code does not even compile on IAR with ""nsapi.dns-cache-size": 0", so some kind of conditional compilation is needed:

[Error] nsapi_dns.cpp@113,0: [Pe094]: the size of an array must be greater than zero
[ERROR] 
  static DNS_CACHE *dns_cache[MBED_CONF_NSAPI_DNS_CACHE_SIZE];

@0xc0170
Copy link
Contributor

0xc0170 commented Jun 20, 2018

This will need to be rebased on that other PR #7207, but seems fine to me.

Can you rebase? There is a conflict now

@cmonr
Copy link
Contributor

cmonr commented Jun 28, 2018

@TeroJaasko Would you mind rebasing?

Setting ""nsapi.dns-cache-size": 0" still left some of the DNS
caching code in. Add crude #if to remove all of it.
This allows one to save 429 bytes of flash and 48 bytes of RAM on
ARMC5 builds.
@TeroJaasko TeroJaasko dismissed stale reviews from mikaleppanen, kjbracey, and SeppoTakalo via 2e89fa2 June 29, 2018 16:05
@TeroJaasko TeroJaasko force-pushed the let_config_remove_dns_cache_code branch from 88d6dc7 to 2e89fa2 Compare June 29, 2018 16:05
@TeroJaasko
Copy link
Contributor Author

Rebased with current master and force pushed.

@cmonr
Copy link
Contributor

cmonr commented Jun 30, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jun 30, 2018

Build : SUCCESS

Build number : 2487
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7239/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Jun 30, 2018

@mbed-ci
Copy link

mbed-ci commented Jun 30, 2018

@0xc0170 0xc0170 merged commit 20adbf0 into ARMmbed:master Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants