Make some traits iterative using static foreach by nordlow · Pull Request #6402 · dlang/phobos

nordlow · 2018-04-01T12:46:52Z

This reduces the number of template instantiations needed and is measured to be at least as fast as the recursive variants here.

The refactored traits are: genericIndexOf, allSatisfy, anySatisfy, isExpression and allSameType.

There a probably more traits that could benefit from this refactoring. Proposals are welcome.

if we added a static break; to exit a static foreach prematurely, would that speed up the compilation even further, @WalterBright?

dlang-bot · 2018-04-01T12:46:54Z

Thanks for your pull request and interest in making D better, @nordlow! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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 fetch digger
dub run digger -- build "master + phobos#6402"

wilzbach · 2018-04-01T21:45:10Z

if we added a static break; to exit a static foreach prematurely, would that speed up the compilation even further

Yeah that would be really cool.
FWIW this was even proposed in DIP1010:

https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1010.md#static-break-and-static-continue

It also comes up in the NG from time to time:

https://forum.dlang.org/post/mailman.2099.1515541202.9493.digitalmars-d-learn@puremagic.com

wilzbach

Nice work!
Interestingly this triggers an ICE when building the docs for std.meta (see DAutoTest)

wilzbach · 2018-04-01T21:45:45Z

std/meta.d

 // [internal]
 private template genericIndexOf(args...)
-if (args.length >= 1)
+    if (args.length >= 1)


std/meta.d(304:9)[warn]: If constraints should have the same indentation as the function

wilzbach · 2018-04-01T21:47:02Z

std/meta.d

-            enum index = 0;
-        }
-        else
+        static if (is(typeof(genericIndexOf) == void) && // not yet defined


Nice trick 👍

wilzbach · 2018-04-01T21:48:53Z

std/meta.d

-    alias tuple = args[1 .. $];
-
-    static if (tuple.length)
+    static foreach (idx, argi; args[1 .. $])


Nit: Why not "arg" instead of "args" (it's a typical convention to denote the array with a "s" postfix.

wilzbach · 2018-04-01T21:50:15Z

std/traits.d

+    static foreach (Ti; T)
+    {
+        static if (!is(typeof(isExpressions) == bool) && // not yet defined
+                   !is(T[0]) && __traits(compiles, { auto ex = T[0]; }))


Shouldn't this be Ti here?

Correct. Fixed. Thanks!

andralex

please mind @wilzbach 's review

andralex · 2018-04-01T14:16:22Z

std/meta.d

 // [internal]
 private template genericIndexOf(args...)
-if (args.length >= 1)
+    if (args.length >= 1)


we keep constraints flush with the declaration they constrain

andralex · 2018-04-02T20:03:21Z

std/meta.d

-            enum index = 0;
-        }
-        else
+        static if (is(typeof(genericIndexOf) == void) && // not yet defined


nordlow

All comments fixed.

nordlow · 2018-04-03T22:32:48Z

std/meta.d

 // [internal]
 private template genericIndexOf(args...)
-if (args.length >= 1)
+    if (args.length >= 1)


nordlow · 2018-04-03T22:32:54Z

std/meta.d

 // [internal]
 private template genericIndexOf(args...)
-if (args.length >= 1)
+    if (args.length >= 1)


nordlow · 2018-04-03T22:33:30Z

std/meta.d

-    alias tuple = args[1 .. $];
-
-    static if (tuple.length)
+    static foreach (idx, argi; args[1 .. $])


nordlow · 2018-04-03T22:33:38Z

std/meta.d

-            enum index = 0;
-        }
-        else
+        static if (is(typeof(genericIndexOf) == void) && // not yet defined


nordlow · 2018-04-03T22:35:42Z

std/traits.d

+    static foreach (Ti; T)
+    {
+        static if (!is(typeof(isExpressions) == bool) && // not yet defined
+                   !is(T[0]) && __traits(compiles, { auto ex = T[0]; }))


Correct. Fixed. Thanks!

nordlow · 2018-04-03T22:45:26Z

Should be ready now. Can you find any other traits that can be made iterative?

Found one: allSameType...hang on...I'll make that iterative too.

nordlow · 2018-04-03T23:10:56Z

Ready now!

wilzbach · 2018-04-03T23:23:04Z

std/traits.d

+    static if (!is(typeof(allSameType) == bool))
    {
-        enum bool allSameType = is(T[0] == T[1]) && allSameType!(T[1..$]);
+        enum bool allSameType = true;


Hmm looks like DScanner's check is bogus here:

std/traits.d(8419:19)[warn]: Variable "allSameType" has the same name as a variable defined on line 8414.

I don't know why the other very similar code is accepted though.
However, you can ignore this module for this check by adding the following to the ModuleFilter blacklist in the .dscanner.ini

label_var_same_name_check="-std.traits"

(Ideally preceded by a reference to a DScanner issue)

nordlow · 2018-04-03T23:39:11Z

Thanks, @wilzbach.

nordlow · 2018-04-03T23:56:44Z

I don't know why Win32 fails...

andralex · 2018-04-04T00:09:08Z

@nordlow try wine

wilzbach · 2018-04-04T03:04:13Z

There's an ICE on Linux too (when run with -D):

core.exception.AssertError@dmd/cond.d(378): Assertion failure
----------------
??:? _d_assertp [0x74903d]
dmd/cond.d:378 pure nothrow @nogc @safe void dmd.cond.StaticForeach.prepare(dmd.dscope.Scope*).__require() [0x51392f]
dmd/cond.d:377 void dmd.cond.StaticForeach.prepare(dmd.dscope.Scope*) [0x5137dc]
dmd/attrib.d:1083 _ZN24StaticForeachDeclaration7includeEP5Scope [0x50c2df]
dmd/doc.d:1042 _ZN11emitComment11EmitComment5visitEP17AttribDeclaration [0x55811b]
dmd/parsetimevisitor.d:75 _ZN16ParseTimeVisitorI10ASTCodegenE5visitEP24StaticForeachDeclaration [0x6437f2]
dmd/attrib.d:1143 _ZN24StaticForeachDeclaration6acceptEP7Visitor [0x50c48d]
dmd/doc.d:1091 void dmd.doc.emitComment(dmd.dsymbol.Dsymbol, dmd.root.outbuffer.OutBuffer*, dmd.dscope.Scope*) [0x557798]
dmd/doc.d:796 void dmd.doc.emitMemberComments(dmd.dsymbol.ScopeDsymbol, dmd.root.outbuffer.OutBuffer*, dmd.dscope.Scope*) [0x55767c]
dmd/doc.d:916 _ZN11emitComment11EmitComment4emitEP5ScopeP7DsymbolPKc [0x557c57]
dmd/doc.d:960 _ZN11emitComment11EmitComment5visitEP11Declaration [0x557ded]
dmd/parsetimevisitor.d:47 _ZN16ParseTimeVisitorI10ASTCodegenE5visitEP16AliasDeclaration [0x643492]
dmd/declaration.d:929 _ZN16AliasDeclaration6acceptEP7Visitor [0x535d61]
dmd/doc.d:1091 void dmd.doc.emitComment(dmd.dsymbol.Dsymbol, dmd.root.outbuffer.OutBuffer*, dmd.dscope.Scope*) [0x557798]
dmd/doc.d:796 void dmd.doc.emitMemberComments(dmd.dsymbol.ScopeDsymbol, dmd.root.outbuffer.OutBuffer*, dmd.dscope.Scope*) [0x55767c]
dmd/doc.d:439 _Z10gendocfileP6Module [0x556783]
dmd/mars.d:911 int dmd.mars.tryMain(ulong, const(char)**) [0x5f8097]
dmd/mars.d:1098 _Dmain [0x5f8d92]

reported -> https://issues.dlang.org/show_bug.cgi?id=18721

wilzbach · 2018-04-04T03:05:42Z

@nordlow you could also try to split this PR into smaller ones, e.g. the documentation ICE comes from allSameType

wilzbach · 2018-04-04T04:39:40Z

Oh and I think the persistent Windows access violation means that there are now too many symbols in the OMF object file (the maximum is 32768).
So I assume another "split-up" of phobos.lib is required :/
See also: https://issues.dlang.org/show_bug.cgi?id=4904

Copyright (C) Digital Mars 2000-2007

We really need to switch to a more modern, actively maintained linker ...

nordlow · 2018-04-04T11:11:59Z

@wilzbach, can I run ddoc generation on phobos via a CLI-call to `make``?

wilzbach · 2018-04-04T11:16:29Z

@wilzbach, can I run ddoc generation on phobos from the makefile?

Well you need to clone dlang.org first, e.g.

cd ~/dlang
git clone https://github.com/dlang/dlang.org
make -f posix.mak phobos-prerelease -j4

and then check web/phobos-prerelease

Though for the failure you don't need any macros, just dmd -D <file> should be enough.

reported -> https://issues.dlang.org/show_bug.cgi?id=18721

BTW my noobish attempt: dlang/dmd#8128 (though the ICE I reduced looks a bit different)

andralex · 2018-04-22T06:48:22Z

status?

nordlow · 2018-04-22T08:26:27Z

Made a merge against master to see if something has changed (for the better).

@andralex, see @wilzbach comments regarding too many symbois in the OMF symbols file. I assume a split of this PR will still (in the end) result in the same Win32-only linking problem, right? @WalterBright ?

nordlow · 2018-10-23T17:41:09Z

I did a merge against master to see if the linking error still occurs on Windows. What's the best way of fixing the too-many-symbol-problem @wilzbach mentioned above?

nordlow · 2018-10-23T18:51:34Z

Build passes. Ok to merge @andralex?

wilzbach · 2018-10-23T19:17:19Z

Please rebase instead of merging for a slicker git history.

wilzbach · 2018-10-23T19:18:54Z

Regarding the linker error: looks like you are lucky and symbols have been moved around in the last six months. As long as auto-tester is green, everything should be okay.

nordlow · 2018-10-23T19:23:37Z

Great @wilzbach. Rebase done.

nordlow · 2018-10-23T19:25:13Z

.dscanner.ini

 ;imports_sortedness="-etc.c.curl,-std.algorithm.comparison,-std.algorithm.internal,-std.algorithm.iteration,-std.algorithm.mutation,-std.algorithm.searching,-std.algorithm.setops,-std.algorithm.sorting,-std.array,-std.bigint,-std.bitmanip,-std.c.freebsd.socket,-std.c.linux.pthread,-std.c.process,-std.complex,-std.concurrency,-std.container.array,-std.container.binaryheap,-std.container.dlist,-std.container.rbtree,-std.container.slist,-std.container.util,-std.conv,-std.datetime,-std.datetime.date,-std.datetime.interval,-std.datetime.systime,-std.datetime.timezone,-std.digest,-std.digest.hmac,-std.exception,-std.experimental.allocator,-std.experimental.allocator.building_blocks,-std.experimental.allocator.building_blocks.affix_allocator,-std.experimental.allocator.building_blocks.allocator_list,-std.experimental.allocator.building_blocks.free_list,-std.experimental.allocator.building_blocks.free_tree,-std.experimental.allocator.building_blocks.kernighan_ritchie,-std.experimental.allocator.building_blocks.region,-std.experimental.allocator.common,-std.experimental.allocator.mallocator,-std.experimental.allocator.mmap_allocator,-std.experimental.allocator.showcase,-std.experimental.allocator.typed,-std.experimental.checkedint,-std.experimental.logger.core,-std.experimental.typecons,-std.file,-std.format,-std.functional,-std.getopt,-std.internal.math.biguintcore,-std.internal.test.dummyrange,-std.json,-std.math,-std.meta,-std.mmfile,-std.net.curl,-std.net.isemail,-std.numeric,-std.outbuffer,-std.parallelism,-std.path,-std.process,-std.random,-std.range,-std.range.primitives,-std.regex,-std.regex.internal.backtracking,-std.regex.internal.generator,-std.regex.internal.kickstart,-std.regex.internal.parser,-std.regex.internal.tests,-std.signals,-std.socket,-std.stdio,-std.string,-std.uni,-std.utf,-std.uuid,-std.variant,-std.windows.charset,-std.windows.registry,-std.windows.syserror,-std.zip"
 ; Checks for labels with the same name as variables
-label_var_same_name_check="-std.algorithm.iteration,-std.algorithm.sorting,-std.array,-std.bigint,-std.bitmanip,-std.conv,-std.encoding,-std.experimental.allocator.building_blocks.segregator,-std.experimental.typecons,-std.format,-std.internal.digest.sha_SSSE3,-std.parallelism,-std.process,-std.typecons,-std.utf"
+label_var_same_name_check="-std.algorithm.iteration,-std.algorithm.sorting,-std.array,-std.bigint,-std.bitmanip,-std.conv,-std.encoding,-std.experimental.allocator.building_blocks.segregator,-std.experimental.typecons,-std.format,-std.internal.digest.sha_SSSE3,-std.parallelism,-std.process,-std.typecons,-std.utf,-std.traits"


Is this still needed @wilzbach ?

I can't recall that this would have been fixed on the dscanner side. Though opening an issue there might help.

Ok, ok to keep as is then so we can pull this?

andralex

Great work!

nordlow · 2018-10-24T07:21:09Z

Thanks.

nordlow · 2018-10-24T08:26:17Z

We really need to switch to a more modern, actively maintained linker ...

Will this keep on hindering us in development?

Port more efficient allSatisfy/anySatisfy from dlang/phobos#6402 merged-on-behalf-of: Sebastian Wilzbach <sebi.wilzbach@gmail.com>

Hackerpilot · 2019-01-08T22:39:20Z

This introduced an undocumented change: https://issues.dlang.org/show_bug.cgi?id=19560

I'm not really sure that I want to call it a regression because the new behavior makes more sense than the old, and the old behavior wasn't really specified.

andralex · 2019-01-08T23:49:07Z

Good catch. I think what was not specified and becomes specified should be documented carefully but not count as a regression.

nordlow requested review from MetaLang, PetarKirov and dnadlinger as code owners April 1, 2018 12:46

wilzbach reviewed Apr 1, 2018

View reviewed changes

andralex approved these changes Apr 2, 2018

View reviewed changes

nordlow force-pushed the iterative-traits branch from b7df9de to 2240075 Compare April 3, 2018 22:43

nordlow commented Apr 3, 2018

View reviewed changes

nordlow force-pushed the iterative-traits branch from 2240075 to 486d7e0 Compare April 3, 2018 23:08

wilzbach reviewed Apr 3, 2018

View reviewed changes

wilzbach added the Severity:Optimization label Apr 3, 2018

nordlow force-pushed the iterative-traits branch from 486d7e0 to a5cb835 Compare April 3, 2018 23:35

wilzbach added the Merge:auto-merge label Apr 3, 2018

wilzbach removed the Merge:auto-merge label Apr 4, 2018

make some traits iterative using static foreach

fc96b0a

nordlow force-pushed the iterative-traits branch from 4d3b9bf to fc96b0a Compare October 23, 2018 19:23

nordlow commented Oct 23, 2018

View reviewed changes

wilzbach approved these changes Oct 23, 2018

View reviewed changes

wilzbach added the Merge:auto-merge label Oct 23, 2018

andralex approved these changes Oct 23, 2018

View reviewed changes

dlang-bot merged commit 6c448be into dlang:master Oct 23, 2018

n8sh added a commit to n8sh/druntime that referenced this pull request Oct 25, 2018

Port more efficient allSatisfy/anySatisfy from dlang/phobos#6402

6eb0f99

n8sh mentioned this pull request Oct 25, 2018

Port more efficient allSatisfy/anySatisfy from dlang/phobos#6402 dlang/druntime#2338

Merged

n8sh added a commit to n8sh/druntime that referenced this pull request Oct 25, 2018

Port more efficient allSatisfy/anySatisfy from dlang/phobos#6402

523db22

dlang-bot added a commit to dlang/druntime that referenced this pull request Oct 25, 2018

Merge pull request #2338 from n8sh/port-phobos-6402

f7c61c5

Port more efficient allSatisfy/anySatisfy from dlang/phobos#6402 merged-on-behalf-of: Sebastian Wilzbach <sebi.wilzbach@gmail.com>

nordlow deleted the iterative-traits branch October 31, 2018 09:44

Uh oh!

Conversation

nordlow commented Apr 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Apr 1, 2018

Bugzilla references

Testing this PR locally

Uh oh!

wilzbach commented Apr 1, 2018

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nordlow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nordlow commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nordlow commented Apr 3, 2018

Uh oh!

wilzbach Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nordlow commented Apr 3, 2018

Uh oh!

nordlow commented Apr 3, 2018

Uh oh!

andralex commented Apr 4, 2018

Uh oh!

wilzbach commented Apr 4, 2018

Uh oh!

wilzbach commented Apr 4, 2018

Uh oh!

wilzbach commented Apr 4, 2018

Uh oh!

nordlow commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nordlow commented Apr 1, 2018 •

edited

Loading

nordlow commented Apr 3, 2018 •

edited

Loading

wilzbach Apr 3, 2018 •

edited

Loading

nordlow commented Apr 4, 2018 •

edited

Loading

nordlow commented Apr 22, 2018 •

edited

Loading

nordlow commented Oct 23, 2018 •

edited

Loading

nordlow Oct 23, 2018 •

edited

Loading