std.regex: major internal redesign, also fixes issue 13532 #5722

DmitryOlshansky · 2017-09-05T07:46:51Z

Finally I pulled this one off :

immutable Regex!Char works
StaticRegex is just a Regex alias
enum vs static ctRegex doesn't matter
template bloat is cut by a decent percent - match***/replace*** functions no longer templated by engine type nor StaticRegex is a distinct type
finally single point of managing intrusive ref-counting of Engines

More importantly this opens up future enhancements:

regex objects now contain a factory that produces optimal matcher for this pattern, the engine choice now can be completely adaptive (each ctRegex has a unique instance of such factory). Now I can special case the heck out of common patterns w/o degrading the design of the library a bit.
less of good ol' messy code, which eventually should bring more contributions to std.regex

dlang-bot · 2017-09-05T07:46:53Z

Thanks for your pull request, @DmitryOlshansky!

Bugzilla references

Auto-close	Bugzilla	Description
✓	13532	std.regex performance (enums; regex vs ctRegex)

DmitryOlshansky · 2017-09-08T08:38:44Z

@wilzbach
89.774% (-0.001%) compared to 7c82e60

Is hillarious. Can we at least set some kind of tolerance of say about 0.01% ?

wilzbach · 2017-09-08T11:11:36Z

Is hillarious. Can we at least set some kind of tolerance of say about 0.01% ?

Sadly AFAIK CodeCov doesn't support this. I was working on querying the API from dlang bot and sending a similar CI status which is in our control.
However, sadly I ran out of time.

DmitryOlshansky · 2017-09-25T08:53:58Z

Wo-ho. Jenkins test succeeded. With auto-tester tackled, I think I'm done here.
Ping @wilzbach

UPDATE: still has bugs on Win32.. working on it.

wilzbach

While I am not very familiar with std.regex, but most changes are rather trivial.
One a first pass, I only found a couple of nits, but it would be great if someone experienced with std.regex could have an eye on this overall big change as well.

wilzbach · 2017-09-26T22:19:06Z

std/regex/internal/backtracking.d

@@ -716,7 +718,7 @@ template BacktrackingMatcher(bool CTregex)
            debug(std_regex_matcher) writeln("pop array SP= ", lastState);
        }

-        static if (!CTregex)
+        static if (true)


What prevents you from removing the static if entirely?

Ehm. Will revisit this

I think that's the only thing missing. If you don't feel like trying to push this we could, of course, also remove this afterwards in a separate PR

wilzbach · 2017-09-26T22:37:59Z

std/regex/internal/backtracking.d

        {
+            _refCount = 1;
+            re = program;


How about moving these two lines to initialize? They are repeated in all constructors ...

re is const so can't assign to it outside of constructor. A very unpleasant limitation

wilzbach · 2017-09-26T22:40:29Z

std/regex/internal/backtracking.d

-        charsets = re.charsets;
+        foreach (ref set; re.charsets)
+        {
+            charsets ~= set.intervals;


@UplinkCoder will be happy about all this allocation at CTFE ;-)

Well, it's the number of character classes in the regex pattern. I hope it's in range of 10s-100s.

what about

auto oldLength = charsets.length; charsets.length += re.charsets.length charsets[oldLength .. $] = re.charsets[];

@stefan-koch-sociomantic Not bad, honestly this piece of code isn't anywhere near top of performance sensitive functions. Trie construction is.
See code around this line:
https://github.com/dlang/phobos/blob/master/std/uni.d#L3871

wilzbach · 2017-09-26T22:45:50Z

std/regex/internal/tests.d

+    import std.conv : to;
+    enum re1 = ctRegex!`[0-9][0-9]`;
+    immutable static re2 = ctRegex!`[0-9][0-9]`;
+    immutable iterations = 1000_000;


Nit: 1_000_000

wilzbach · 2017-09-26T22:47:01Z

std/regex/internal/tests.d

+    assert(result1 == result2);
+    auto ratio = 1.0 * enumTime.total!"usecs" / staticTime.total!"usecs";
+    // enum is faster or the diff is less < 30%
+    assert(ratio < 1.0 || abs(ratio - 1.0) < 0.3,


Seeing that you experimented quite a bit with this, this still seems a bit fragile.

And it is. It seems I can't provide even basic performance guarantees in the unittest.
This is a problem

wilzbach · 2017-09-26T22:58:34Z

std/regex/internal/ir.d

+        return ++m.refCount;
+    }
+
+    override size_t decRef(Matcher!Char m) const  @trusted


Why not use postblit this(this) and destructor ~this() instead of rolling your own mechanism?

Ploymorphism! structs are not polymorphic, this class is then wrapped in a proper struct that does the inc/dec

wilzbach · 2017-09-26T22:58:56Z

std/regex/internal/ir.d

+    // This only maintains internal ref-count,
+    // deallocation happens inside MatcherFactory
+    @property ref size_t refCount() @safe;
+    // Copy internal state to another engine, using memory arena 'memory'


Arena? Did you mean "zone" or "region"?
https://en.wikipedia.org/wiki/Region-based_memory_management

I really do not care.. a chunk of memory it is

@wilzbach area or region it's all the same.

Yes, but "arena" sounded strange. Note that I don't care about it, it just struck me while reading the diff

wilzbach · 2017-09-26T23:00:54Z

std/regex/internal/parser.d

+        // check if we have backreferences, if so - use backtracking
+        if (__ctfe) factory = null; // allows us to use the awful enum re = regex(...);
+        else
+        if (re.backrefed.canFind!"a != 0")


Nit: Common Phobos style is to have else if in the same line

wilzbach · 2017-09-26T23:12:35Z

std/regex/internal/ir.d

+    {
+        auto r = cast() this;
+        r.factory = factory;
+        return r;


While this is a nice pattern, you completely work around const here.
At least it's internal ;-)

Yes. I feel like a compiler can prove this is legal iff all member of struct are value types or const references.

wilzbach · 2017-09-26T23:17:19Z

std/regex/package.d

@@ -1530,15 +1468,15 @@ private:
    @trusted this(Range input, RegEx separator)
    {//@@@BUG@@@ generated opAssign of RegexMatch is not @trusted
        _input = input;
-        separator.flags |= RegexOption.global;
+        auto re = separator.withFlags(separator.flags | RegexOption.global);


s/auto/const

Disable "benchmark" in unittest, it's too volatile with different compiler flags Also use GC.addRange/GC.removeRange

wilzbach · 2017-10-06T16:15:53Z

Running DScanner
../dscanner-285ef162f024cbd305d587e9e0fcfb2292ea93ce/dsc --config .dscanner.ini --styleCheck etc std -I.
make: *** [dscanner] Segmentation fault (core dumped)
@wilzbach My code is so ugly?

Nay, sadly this random segfault has been appearing in the last two weeks. I nor anyone else hasn't had time to dive into. However, it only happens for every third run ...

DmitryOlshansky · 2017-10-08T14:10:48Z

Alright, I think I addressed all of comments.

@CyberShadow @wilzbach @ZombineDev
What do you think?

andralex · 2017-10-11T08:50:35Z

So why does this fail jenkins and codecov?

stefan-koch-sociomantic · 2017-10-11T08:54:13Z

I'd say because it sees comments as uncovered code ?

DmitryOlshansky · 2017-10-11T11:13:38Z

So why does this fail jenkins and codecov?

Jenkin passes just fine. Codecov is a mess and doesn't work with std.regex package structure.

wilzbach · 2017-10-11T13:30:30Z

Codecov is a mess and doesn't work with std.regex package structure.

Jup sadly that's true, but it's only intended as tool to help reviewers anyhow.

wilzbach

I think we should let @DmitryOlshansky move forward with this. The only thing I saw was the static if (true), but than one can be addressed later as well...

DmitryOlshansky · 2017-10-11T14:23:56Z

Jup sadly that's true, but it's only intended as tool to help reviewers anyhow.

Should somehow indicate that it's optional. The common wisdom is red cross - no merge.

andralex · 2017-10-11T14:21:14Z

std/regex/internal/backtracking.d

-            tmp.initExternalMemory(memory);
-            return tmp;
+            auto backtracking = cast(BacktrackingMatcher) m;
+            backtracking.s = s;


with (backtracking) comes to mind

but then

with(backtracking) { s = s; }

???

yah one name would need to be changed :)

DmitryOlshansky · 2017-10-11T16:30:03Z

@wilzbach
This, again...

../dscanner-285ef162f024cbd305d587e9e0fcfb2292ea93ce/dsc --config .dscanner.ini --styleCheck etc std -I.
std/regex/package.d(432:49)[warn]: Parameter matcher is never used.
make: *** [dscanner] Error 1

The code looks like this:

@trusted bool func(BacktrackingMatcher!Char matcher)
    {
        debug(std_regex_ctr) pragma(msg, source);
        mixin(source);
    }

I'm f**king tired of optimistic checks landing into our CI infrastructure.
If you can't prove something - don't make that check mandatory!

MartinNowak · 2017-12-29T10:39:56Z

Great stuff, looks like it regressed our funky dlang-bot regex-split-joiner again.
18135 – [REG2.078] can't join RegexMatch anymore
Trying to reduce the test case.

MartinNowak · 2018-01-04T21:49:20Z

There is also a reported performance regression @DmitryOlshansky.
18114 – [Reg 2.078] regex performance regression

dlang-bot added the Enhancement label Sep 5, 2017

DmitryOlshansky force-pushed the regex-matcher-interfaces branch 2 times, most recently from b3ca451 to dc32bbe Compare September 5, 2017 10:48

DmitryOlshansky added Bug Fix and removed Bug Fix labels Sep 5, 2017

DmitryOlshansky force-pushed the regex-matcher-interfaces branch 9 times, most recently from 671a5f7 to b6bfd87 Compare September 6, 2017 09:46

DmitryOlshansky force-pushed the regex-matcher-interfaces branch from 4f8f3e3 to 542d1b8 Compare September 25, 2017 08:53

DmitryOlshansky requested review from JackStouffer, PetarKirov and wilzbach September 25, 2017 12:15

wilzbach reviewed Sep 26, 2017

View reviewed changes

DmitryOlshansky added 7 commits September 28, 2017 11:55

WIP - refactoring towards sane std.regex code

6e68434

Finalized matcher factory abstraction, with compile-time regex support

33d7ecc

The new std.regex design is concluded, going through the unitests

639dd3d

std.regex finish the loose ends in the great refactoring

c7bdfbe

Fix issue 13532 - std.regex performance (enums; regex vs ctRegex)

a877469

Fix CI complaints

8bfa66c

Fix latent bug due to emplacing over uninitialized memory

eaa62a8

DmitryOlshansky force-pushed the regex-matcher-interfaces branch from 634883f to 5b3e698 Compare October 6, 2017 07:59

Need to propagate subCounters

41c2296

Disable "benchmark" in unittest, it's too volatile with different compiler flags Also use GC.addRange/GC.removeRange

DmitryOlshansky force-pushed the regex-matcher-interfaces branch from 5b3e698 to 41c2296 Compare October 6, 2017 08:23

wilzbach approved these changes Oct 11, 2017

View reviewed changes

andralex approved these changes Oct 11, 2017

View reviewed changes

andralex added the auto-merge label Oct 11, 2017

Drop another level of templatizations in backtracking

13ea5f9

DmitryOlshansky force-pushed the regex-matcher-interfaces branch from 886d99c to 13ea5f9 Compare October 11, 2017 15:20

dlang-bot removed the auto-merge label Oct 11, 2017

DmitryOlshansky added the auto-merge label Oct 11, 2017

dlang-bot removed the auto-merge label Oct 12, 2017

DmitryOlshansky force-pushed the regex-matcher-interfaces branch 2 times, most recently from 0f86e7d to a292141 Compare October 16, 2017 08:02

Silence the circleCI style check?

f4c963b

DmitryOlshansky force-pushed the regex-matcher-interfaces branch from a292141 to f4c963b Compare October 16, 2017 16:16

DmitryOlshansky added the auto-merge label Oct 16, 2017

dlang-bot merged commit ad48998 into dlang:master Oct 16, 2017

This was referenced Feb 13, 2018

Merge 2.078.1 front-end and stdlibs ldc-developers/ldc#2486

Merged

std.regex: Alignment hotfix #6185

Merged

MartinNowak mentioned this pull request Feb 22, 2018

env var replacement not supporting ${VAR_NAME} syntax dlang/dub#1392

Merged

std.regex: major internal redesign, also fixes issue 13532 #5722

std.regex: major internal redesign, also fixes issue 13532 #5722

Conversation

DmitryOlshansky commented Sep 5, 2017 • edited Loading

dlang-bot commented Sep 5, 2017

Bugzilla references

DmitryOlshansky commented Sep 8, 2017

wilzbach commented Sep 8, 2017

DmitryOlshansky commented Sep 25, 2017 • edited Loading

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitryOlshansky Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitryOlshansky Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Oct 6, 2017

DmitryOlshansky commented Oct 8, 2017

andralex commented Oct 11, 2017

stefan-koch-sociomantic commented Oct 11, 2017

DmitryOlshansky commented Oct 11, 2017 • edited Loading

wilzbach commented Oct 11, 2017

wilzbach left a comment

Choose a reason for hiding this comment

DmitryOlshansky commented Oct 11, 2017

Choose a reason for hiding this comment

DmitryOlshansky Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DmitryOlshansky commented Oct 11, 2017

MartinNowak commented Dec 29, 2017

MartinNowak commented Jan 4, 2018

DmitryOlshansky commented Sep 5, 2017 •

edited

Loading

DmitryOlshansky commented Sep 25, 2017 •

edited

Loading

DmitryOlshansky Sep 27, 2017 •

edited

Loading

DmitryOlshansky Sep 27, 2017 •

edited

Loading

DmitryOlshansky commented Oct 11, 2017 •

edited

Loading

DmitryOlshansky Oct 11, 2017 •

edited

Loading