Make -lowmem optional and prefer building d_do_test with the host compiler

[wilzbach]

CC @MoonlightSentinel

[dlang-bot]

Thanks for your pull request, @wilzbach!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	21053	blocker	Test Suite run.d is built with compiler under test, must be build with bootstrap dmd

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

1. Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

2. Change the base branch of your PR to stable

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 + dmd#11519"

[MoonlightSentinel]

Interesting ... wasn't -lowmem introduced because of an OOM on the auto-tester or was it a different CI?

[wilzbach]

Interesting ... wasn't -lowmem introduced because of an OOM on the auto-tester or was it a different CI?

Yeah, very good point - f9a4e89

[wilzbach]

Interesting ... wasn't -lowmem introduced because of an OOM on the auto-tester or was it a different CI?

Yeah, very good point - f9a4e89

It's still the case:

[kinke]

Interesting ... wasn't -lowmem introduced because of an OOM on the auto-tester or was it a different CI?

Yeah, very good point - f9a4e89

It's still the case:

Yeah, but at that time I've also enabled parallel building of d_do_test unittests + normal executable, which isn't parallelized anymore.

[WalterBright]

The test failure here is:

Test fail_compilation/diag10327.d failed: 
expected:
----
fail_compilation/diag10327.d(11): Error: module `test10327` is in file 'imports/test10327.d' which cannot be read
import path[0] = fail_compilation
import path[1] = $p:druntime/import$
import path[2] = $p:phobos$
----
actual:
----
fail_compilation/diag10327.d(11): Error: module `test10327` is in file 'imports/test10327.d' which cannot be read
import path[0] = fail_compilation
import path[1] = /Users/braddr/sandbox/at-client/pull-4136150-Darwin_64_64/dmd/test/../../druntime/import
import path[2] = /Users/braddr/sandbox/at-client/pull-4136150-Darwin_64_64/dmd/test/../../phobos
----
diff:
----
 fail_compilation/diag10327.d(11): Error: module `test10327` is in file 'imports/test10327.d' which cannot be read
 import path[0] = fail_compilation
-import path[1] = $p:druntime/import$
-import path[2] = $p:phobos$
+import path[1] = /Users/braddr/sandbox/at-client/pull-4136150-Darwin_64_64/dmd/test/../../druntime/import
+import path[2] = /Users/braddr/sandbox/at-client/pull-4136150-Darwin_64_64/dmd/test/../../phobos
----

[MoonlightSentinel]

@wilzbach The main memory requirement stems from the CTFE done for regex. We could disable it without -lowmem using something like this:

diff --git a/test/tools/d_do_test.d b/test/tools/d_do_test.d
index aecb61173..14111c8c1 100755
--- a/test/tools/d_do_test.d
+++ b/test/tools/d_do_test.d
@@ -613,13 +613,14 @@ string unifyNewLine(string str)
     // On Windows, Outbuffer.writenl() puts `\r\n` into the buffer,
     // then fprintf() adds another `\r` when formatting the message.
     // This is why there's a match for `\r\r\n` in this regex.
-    static re = regex(`\r\r\n|\r\n|\r|\n`, "g");
+    static re = buildRegex(`\r\r\n|\r\n|\r|\n`, "g");
     return std.regex.replace(str, re, "\n");
 }
 
 string unifyDirSep(string str, string sep)
 {
-    static re = regex(`(?<=[-\w{}][-\w{}]*)/(?=[-\w][-\w/]*\.(di?|mixin)\b)`, "g");
+    static re = buildRegex(`(?<=[-\w{}][-\w{}]*)/(?=[-\w][-\w/]*\.(di?|mixin)\b)`, "g");
+
     return std.regex.replace(str, re, sep);
 }
 unittest
@@ -1662,3 +1663,26 @@ void printCppSources (in const(char)[][] compiled)
         }
     }
 }
+
+version (LazyRegex)
+{
+    struct buildRegex
+    {
+        string pattern, flags;
+        typeof(regex(pattern, flags)) re = void;
+
+        auto ref getRegex()
+        {
+            if (pattern !is null)
+            {
+                re = regex(pattern, flags);
+                pattern = null;
+            }
+            return re;
+        }
+
+        alias getRegex this;
+    }
+}
+else
+    alias buildRegex = regex;

Don't think no-CTFE should be the default because it implies a startup penalty for every invocation of d_do_test which could stack up a lot given the amount of individual tests,

[wilzbach]

We could disable it without -lowmem using something like this:

AFAICT the unifyNewLine regular expression isn't very memory expensive (and could be easily replaced with a normal replace function). The unifyDirSep, however, costs about half of the total 1.5GB, so I just wanted to see whether we can roll a more naive version as a function.

I like your approach too and it's definitely the a good fallback if we have to use the regex.
Another easy workaround would be to put unifyNewLine in a separate file, s.t. we can compile it separately and at least have the memory consumption halfed that way.

[wilzbach]

The unifyDirSep, however, costs about half of the total 1.5GB, so I just wanted to see whether we can roll a more naive version as a function.

So it looks like this works for all test suites. It's less elegant than the regular expression, but reduces the memory consumption from 1.5G to 740M (without -lowmem). I kept it rather verbose, s.t. it's easier to understand, but if line-count is an issue, it could be condensed.

[MoonlightSentinel]

Please use Phobos style to keep this code consistent.
Otherwise LGTM.

[wilzbach]

Please use Phobos style to keep this code consistent.

I had another look at this and found a more elegant non-regex approach which should be good enough for this use. Let's see what the CIs say.

[MoonlightSentinel]

Nice but not enough:

-compilable\ddoc_markdown_links_verbose.d(28): Ddoc: found link reference 'dub' to 'https:\\code.dlang.org'
+compilable\ddoc_markdown_links_verbose.d(28): Ddoc: found link reference 'dub' to 'https://code.dlang.org'

Maybe add a lookahead to check that the suffix is an actual file extension?

[wilzbach]

Maybe add a lookahead to check that the suffix is an actual file extension?

Added. Looks like the CIs are happy with it (:

[wilzbach]

CC @thewilsonator. This is ready and as the d_do_test segfaults keep happening randomly, maybe this can get a bit of priority?

[MoonlightSentinel]

To get the CI working for other PR's.

Not sure if there are other edge cases for unifyDirSep but it should work for dmd's error messages AFAICT.