Skip to content

Rewrite std.meta.staticMap using AliasAssign#7756

Closed
WalterBright wants to merge 1 commit intodlang:masterfrom
WalterBright:AliasAssignStaticMap
Closed

Rewrite std.meta.staticMap using AliasAssign#7756
WalterBright wants to merge 1 commit intodlang:masterfrom
WalterBright:AliasAssignStaticMap

Conversation

@WalterBright
Copy link
Member

Try out dlang/dmd#12029

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7756"

@WalterBright
Copy link
Member Author

The error is:

std/conv.d(5710): Error: static assert:  `!__traits(compiles, emplace(& s2, 1))` is false

but I'm not seeing where staticMap is being called?

@WalterBright
Copy link
Member Author

@9il can you please have a look at the buildkite failures in Mir?

@WalterBright
Copy link
Member Author

dscanner fails with the new syntax (buildkite)

@WalterBright
Copy link
Member Author

The buildkite druntime error is:

generated/linux/release/64/unittest/test_runner core.sys.windows.dde
--| generated/linux/release/64/benchmark.o:benchmark/runbench.d:function _D8runbench6runCmdFAyabIAaZQi: error: undefined reference to '_D3std7process12executeShellFNeMAxaxHAyaAyaEQBqQBp6ConfigmMQBbQzZSQCm8typecons__T5TupleTiVQCba6_737461747573TQCuVQCya6_6f7574707574ZQBz'

@WalterBright
Copy link
Member Author

@s-ludwig In buildkite taggedalgebraic I'm getting an error:

source/taggedalgebraic/taggedunion.d(449,13): Error: function `taggedalgebraic.taggedunion.TaggedUnion!(E).TaggedUnion.set!E.text.set()` is not callable using argument types `(string)`

@9il
Copy link
Member

9il commented Jan 20, 2021

@WalterBright

Looks like the following mir-core unittest is failed because of a syntax error (regresssion), that is probably is unrelated to this MR. The CI compiler can't handle static F[3][] vals(F) = ... expression anymore.

@safe @nogc nothrow version(mir_core_test) unittest
{
    import std.meta: AliasSeq;
    static F[3][] vals(F) =    // value,exp,ldexp
    [
    [    0,    0,    0],
    [    1,    0,    1],
    [    -1,    0,    -1],
    [    1,    1,    2],
    [    123,    10,    125952],
    [    F.max,    int.max,    F.infinity],
    [    F.max,    -int.max,    0],
    [    F.min_normal,    -int.max,    0],
    ];
    static foreach(F; AliasSeq!(double, real))
    {{
        int i;

        for (i = 0; i < vals!F.length; i++)
        {
            F x = vals!F[i][0];
            int exp = cast(int) vals!F[i][1];
            F z = vals!F[i][2];
            F l = ldexp(x, exp);
            assert(feqrel(z, l) >= 23);
        }
    }}
}

mir-algorithm is failed because of another thing, I did a new release to so it will print at least what UDA keys are reflected with the new staticMap.

@s-ludwig
Copy link
Member

source/taggedalgebraic/taggedunion.d(449,13): Error: function `taggedalgebraic.taggedunion.TaggedUnion!(E).TaggedUnion.set!E.text.set()` is not callable using argument types `(string)`

It looks like there is a dependency cycle between staticMap!(TypeOf, EnumMembers!FieldEnum) (which is indexed to declare the parameter type for that set method) and the template TypeOf. This works right now, but seems fishy anyway. I'll have to take a closer look tomorrow.

@WalterBright
Copy link
Member Author

@s-ludwig thanks! Though this should behave identically to the old implementation of staticMap, warts and all.

@maxhaton
Copy link
Member

out.pdf

Plot of dumb timings of staticMap!(Alias, int, int, int [up to 8192 times]). Starts off better, scales worse (I assume due to memory usage, even with lowmem the heap sizes are enormous)

I'll do some heavier operations later.

@WalterBright
Copy link
Member Author

@maxhaton Good information. I bet the result is because the old is using a binary method to build long alias sequences, while the new is using the sequential one. Changing the implementation of staticMap to a binary method should fix that.

@maxhaton
Copy link
Member

@WalterBright more perf plots

Same N values for fullyQualifiedName (slower, more memory usage) fullyQualifiedNameData.pdf

First few 150 plotted in more detail fullyQualifiedNameData_linear.pdf

aliasplots_linear.pdf Very clear division in the log-log plot

@WalterBright
Copy link
Member Author

The old staticMap had changed since I last worked with it. Instead of calling itself, it used a function to generate a string that enumerated all the cases, and did one big (up to 150) case AliasSeq. Unfortunately, that implementation traded off the recursive call for a rather large set of compile-time allocations in generating the string.

@WalterBright
Copy link
Member Author

WalterBright commented Jan 21, 2021

@9il @s-ludwig thank you for fixing buildkite!

@9il
Copy link
Member

9il commented Jan 21, 2021

@9il @s-ludwig thank you for fixing buildkite!

Something has been fixed in DMD / Phobos. I didn't really fix anything.

@9il
Copy link
Member

9il commented Jan 21, 2021

I have accidentally edited @WalterBright's comment and was surprised that I am able to do so. That is a bit weird.

@WalterBright
Copy link
Member Author

@9il that's because you have privileged access. It's a superpower, I only used it once when the comment writer asked me to modify it.

@s-ludwig
Copy link
Member

Same here, might be because all of the sequences in those taggedalgebraic unit tests are <= 8 entries and the issue doesn't manifest for the first assignment.

@RazvanN7
Copy link
Collaborator

@WalterBright please rebase? Also, what are the plans with AliasAssign and phobos?

@RazvanN7
Copy link
Collaborator

RazvanN7 commented May 6, 2021

I'm going to close this as #8039 seems to be a superior implementation.

@RazvanN7 RazvanN7 closed this May 6, 2021
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.

6 participants

Comments