Issue 13159 - std.socket.getAddress allocates once per DNS lookup hit
Summary: std.socket.getAddress allocates once per DNS lookup hit
Status: REOPENED
Alias: None
Product: D
Classification: Unclassified
Component: phobos (show other issues)
Version: D2
Hardware: x86_64 Linux
: P4 enhancement
Assignee: No Owner
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-19 12:26 UTC by JR
Modified: 2024-12-01 16:21 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description JR 2014-07-19 12:26:15 UTC
[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.)
Comment 1 Orvid King 2014-07-19 16:11:33 UTC
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.
Comment 2 Jakob Ovrum 2014-07-19 16:25:34 UTC
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.
Comment 3 github-bugzilla 2014-07-20 19:32:17 UTC
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
Comment 4 Vladimir Panteleev 2015-08-29 21:18:45 UTC
???

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?
Comment 5 Vladimir Panteleev 2015-08-29 21:21:59 UTC
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).
Comment 6 Jakob Ovrum 2015-08-29 21:55:15 UTC
(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 :)
Comment 7 dlangBugzillaToGithub 2024-12-01 16:21:59 UTC
THIS ISSUE HAS BEEN MOVED TO GITHUB

https://github.com/dlang/phobos/issues/9637

DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB