std.socket.getAddress allocates once per DNS lookup hit

[dlangBugzillaToGithub]

zorael reported this on 2014-07-19T12:26:15Z

Transfered from https://issues.dlang.org/show_bug.cgi?id=13159

CC List

• blah38621
• dlang-bugzilla (@CyberShadow)
• jakobovrum

Description

[Arch x86_64, dmd/phobos/druntime from git 140719]

std.socket.getAddress is very allocation-happy. A DNS lookup often/(always?) gives more than one resulting IP, and getAddress naïvely concatenates each into an Address[] array in a foreach loop. As a concrete example, getAddress("irc.freenode.net", cast(ushort)6667) returned 51 hits.

In Address[] getAddress(in char[], in char[]) near line 1116:
>  // use getAddressInfo
>  Address[] results;
>  auto infos = getAddressInfo(hostname, service);
>   foreach (ref info; infos)
>       results ~= info.address;
>   return results;

Unless this is a valid use-case for ScopeBuffer, could we at least tack a results.reserve(64) in there?

(I think AddressInfo.sizeof is 40 bytes.)

[dlangBugzillaToGithub]

blah38621 commented on 2014-07-19T16:11:33Z

Assuming that infos has a length property, it would be better yet to simply do:

auto infos = getAddressInfo(hostname, service);
auto results = new Address[infos.length];

Which would result in exactly the amount of memory needed being allocated, and only 1 allocation being done. I would also suggest the possibility of adding an OutputRange based version.

[dlangBugzillaToGithub]

jakobovrum commented on 2014-07-19T16:25:34Z

I already posted a PR for this BTW:

https://github.com/D-Programming-Language/phobos/pull/2351

(In reply to Orvid King from comment #1)
> Assuming that infos has a length property, it would be better yet to simply
> do:
> 
> auto infos = getAddressInfo(hostname, service);
> auto results = new Address[infos.length];

This would allocate even when infos.length is 0.

> Which would result in exactly the amount of memory needed being allocated,
> and only 1 allocation being done.

`getAddressInfo` allocates too, and easily more than once, so that wouldn't be quite true.

> I would also suggest the possibility of adding an OutputRange based version.

We can do better - a version that returns the results as a lazy forward range. The most underlying data structure here is a singly linked list. We can use reference counting to ensure the list is freed (freeaddrinfo). It's a much heavier change though so I'm not going to do it in PR #2351.

[dlangBugzillaToGithub]

github-bugzilla commented on 2014-07-20T19:32:17Z

Commit pushed to master at https://github.com/D-Programming-Language/phobos

https://github.com/D-Programming-Language/phobos/commit/96893cbf467524d05d9f8b0f51398585ca77a423
Pre-allocate result array in getAddress and use Appender in getAddressInfoImpl

Fixes issue #13159

[dlangBugzillaToGithub]

dlang-bugzilla (@CyberShadow) commented on 2015-08-29T21:18:45Z

???

A concatenation is not an allocation! The runtime will effectively increase arrays when they are appended to by powers of two until the GC block size (4096 bytes), because GC object bins have sizes of powers of two (16 to 2048).

Furthermore, didn't recent benchmarks show that std.array.appender performed no better than built-in arrays for concatenation?

[dlangBugzillaToGithub]

dlang-bugzilla (@CyberShadow) commented on 2015-08-29T21:21:59Z

The getAddress patch is fine. The getAddressInfo patch seems pointless to me, it does not preallocate any memory (but could be made to if the linked list is traversed twice).

[dlangBugzillaToGithub]

jakobovrum commented on 2015-08-29T21:55:15Z

(In reply to Vladimir Panteleev from comment #5)
> The getAddress patch is fine. The getAddressInfo patch seems pointless to
> me, it does not preallocate any memory (but could be made to if the linked
> list is traversed twice).

Appender has gone through a lot of revision in recent years. Array appends might be better today.

The difference is probably negligible; the getAddress patch was the main point. The ideal would be a lazy range version of getAddressInfo. With both `getAddress` and `getAddressInfo` taken, bikeshedding ahoy :)