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

Fix IPv6 requests in the HTTP client #2082

Merged
merged 3 commits into from Feb 24, 2018

Conversation

Projects
None yet
3 participants
@s-ludwig
Member

s-ludwig commented Feb 23, 2018

No description provided.

@@ -0,0 +1,32 @@
/++ dub.sdl:
dependency "vibe-d" version="~>0.8.3-alpha.4"

This comment has been minimized.

@wilzbach

wilzbach Feb 23, 2018

Contributor

Shouldn't this be path too? (we probably should allow single files in the test suite too)

This comment has been minimized.

@s-ludwig

s-ludwig Feb 23, 2018

Member

True and true!

} catch (Exception e) assert(false, e.msg);
});
runApplication();
} else logWarn("Skipping HTTP+IPv6 test due to Travis not supporting IPv6 loopback");

This comment has been minimized.

@wilzbach

wilzbach Feb 23, 2018

Contributor

This probably gets forgotten easily. Have you considering removing the test in the travis.sh script?

This comment has been minimized.

@s-ludwig
// Provide port number when it is not the default one (RFC2616 section 14.23)
// IPv6 addresses need to be put into brackets
auto hoststr = url.host.canFind(':') ? "["~url.host~"]" : url.host;

This comment has been minimized.

@wilzbach

wilzbach Feb 23, 2018

Contributor

If you care about performance, choose(url.host.canFind(':'), url.host, chain("[", url.host, "]")) does this without allocation.

This comment has been minimized.

@s-ludwig

s-ludwig Feb 23, 2018

Member

But the result is needed as a string anyway, so until there is range support for writing headers, it wouldn't really save anything.

This comment has been minimized.

@wilzbach

wilzbach Feb 23, 2018

Contributor

format uses an appender internally and thus is probably a tiny bit faster, but yeah I agree that it won't be noticeable and the the real gain will come from not allocating at all.

This comment has been minimized.

@s-ludwig

s-ludwig Feb 23, 2018

Member

AFAIR, multiple ~ expressions are automatically joined efficiently by counting the total length first and allocating only a single buffer. I never verified this, though.

This comment has been minimized.

@wilzbach

wilzbach Feb 23, 2018

Contributor

AFAIR, multiple ~ expressions are automatically joined efficiently by counting the total length first and allocating only a single buffer.

Yes (https://github.com/dlang/druntime/blob/master/src/rt/lifetime.d#L2217), but it's still an allocation.

This comment has been minimized.

@s-ludwig

s-ludwig Feb 24, 2018

Member

But Appender has to allocate, too. Or do you mean something else?

This comment has been minimized.

@wilzbach

wilzbach Feb 24, 2018

Contributor

Yes, but my point was it has to do so in any case at the moment and one big allocation might be a tiny bit faster.
Anyhow, I doubt that it will matter and I wouldn't be too shocked if at the moment the implementation of format would even be slower due to the non-zero cost abstraction of ranges , simply not calling reserve on the appender, the appender itself being slow etc. . Unfortunately, Phobos does a really poor job performance-wise at the moment :/

This comment has been minimized.

@s-ludwig

s-ludwig Feb 24, 2018

Member

I don't get it :) So _d_arraycatnTX does a single big allocation, too (actually an allocation with the exact length, while Appender probably over-allocates?). Why would it be slower than Appender?

This comment has been minimized.

@s-ludwig

s-ludwig Feb 24, 2018

Member

Yeah, but anyway, it's not really important right now. Just the new vibe-http should get a low level API that avoids these kinds of allocations.

This comment has been minimized.

@wilzbach

wilzbach Feb 24, 2018

Contributor

I don't get it :) So _d_arraycatnTX does a single big allocation, too (actually an allocation with the exact length, while Appender probably over-allocates?). Why would it be slower than Appender?

Ah I think we were talking about different things.
I was trying to say that the combination of e.g.

auto s = a ~ "foo" ~ b;
return format("%s%s", ".", s); // <- allocates again with an appender

in a perfect world is slower than

auto s = chain(a, "foo", b);
return format("%s%s", ".", s);

Yeah, but anyway, it's not really important right now.

Yes!

@dlang-bot dlang-bot removed the auto-merge label Feb 23, 2018

@wilzbach wilzbach added the auto-merge label Feb 23, 2018

@wilzbach wilzbach merged commit 570797f into master Feb 24, 2018

4 of 5 checks passed

continuous-integration/appveyor/branch AppVeyor build failed
Details
codecov/patch 69.23% of diff hit (target 58.877%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@wilzbach wilzbach deleted the issue2080_http_client_ipv6 branch Feb 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment