-
-
Notifications
You must be signed in to change notification settings - Fork 739
Dramatically faster to compile Tuple #5931
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
Conversation
Thanks for your pull request, @andralex! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
std/typecons.d
Outdated
@@ -535,14 +535,14 @@ if (distinctFieldNames!(Specs)) | |||
string injectNamedFields() | |||
{ | |||
string decl = ""; | |||
import std.conv : to; | |||
foreach (i, name; staticMap!(extractName, fieldSpecs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
static foreach (i, val; fieldSpecs)
{{
alias name = val.name;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wilzbach done
std/typecons.d
Outdated
foreach (i, name; staticMap!(extractName, fieldSpecs)) | ||
{ | ||
import std.format : format; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. Yeah, std.format
is a bear in CTFE. It's pretty awesome that you can actually use it in CTFE, but performance is thrown out the window.
1e1f070
to
2c70ab5
Compare
Btw another huge slow-down is template parseSpecs(Specs...)
{
static if (Specs.length == 0)
{
alias parseSpecs = AliasSeq!();
}
else static if (is(Specs[0]))
{
// SWAP this with the block below -> _a lot_ faster
// alias parseSpecs = staticMap!FieldSpec;
static if (is(typeof(Specs[1]) : string))
{
alias parseSpecs =
AliasSeq!(FieldSpec!(Specs[0 .. 2]),
parseSpecs!(Specs[2 .. $]));
}
else
{
alias parseSpecs =
AliasSeq!(FieldSpec!(Specs[0]),
parseSpecs!(Specs[1 .. $]));
}
}
else
{
static assert(0, "Attempted to instantiate Tuple with an "
~"invalid argument: "~ Specs[0].stringof);
}
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The auto-tester failures look weird 😦
Before this change:
With this change:
Look at e.g. |
@wilzbach could the autotester failures have something to do with the recent changes in the test infra? |
This PR (dlang/dmd#7444) - submitted an hour ago - is passing on all machines: https://auto-tester.puremagic.com/pull-history.ghtml?projectid=1&repoid=1&pullid=7444 |
@wilzbach Re:
But I've no idea whether the cost of CTFE + mixin is actually any better than just expanding the current template implementation. I suppose for very large specs it might surpass the template expansion, due to less symbols being generated, but that's pure speculation on my part. For small specs, it's a toss-up which one is better. Have to profile to know for sure. |
P.S. Well, the idea behind using |
How do I restart the tester? I tried |
So I can reproduce the failures locally when trying to build the DMD testsuite with Phobos built with this branch :/ |
@wilzbach what command(s) do you run? The things I tried work on my system. |
Are your cd ~/dlang/dmd/test && make clean && make |
@wilzbach eh, I now see it works. What gives? |
You just reverted the commit ... |
3aa216a
to
a59749c
Compare
Oh OK. It seems the import of std.conv was the culprit, likely resulting in exposing a CT bug. I've replaced |
a59749c
to
6e36f67
Compare
On my Airtop i7 with the latest: before (best of 5): 0.286s, after: 0.038s. |
6e36f67
to
5e54cf7
Compare
@andralex: you seem to have amended your work to the revert commit. I rebased your PR and squashed both commits into one. |
@andralex proofs my point about templates. staticMap was the culprit here. |
Investigation following discussion in #5916