Make oniguruma a default regex engine #618

kuba-- · 2019-02-13T15:47:41Z

Signed-off-by: kuba-- kuba@sourced.tech
Closes #615

Integrate hyperscan as a new (default) regex engine.

kuba-- · 2019-02-15T00:30:45Z

.travis.yml

@@ -75,7 +89,7 @@ jobs:
        - make TEST=ruby integration

    - language: java
-      jdk: oraclejdk9
+      jdk: openjdk10


to fix a build

kuba-- · 2019-02-15T00:30:59Z

go.mod

@@ -8,13 +8,15 @@ require (
 	github.com/boltdb/bolt v1.3.1
 	github.com/cespare/xxhash v1.1.0 // indirect
 	github.com/codahale/hdrhistogram v0.0.0-20161010025455-3a0bb77429bd // indirect
+	github.com/flier/gohs v1.0.0


binding for hyperscan

should we update this file?

"go mod tidy"

kuba-- · 2019-02-15T00:31:08Z

go.mod

 	github.com/go-ole/go-ole v1.2.2 // indirect
 	github.com/go-sql-driver/mysql v1.4.1
 	github.com/gogo/protobuf v1.2.0 // indirect
 	github.com/google/go-cmp v0.2.0 // indirect
 	github.com/gorilla/handlers v1.4.0 // indirect
 	github.com/gorilla/mux v1.7.0 // indirect
 	github.com/hashicorp/memberlist v0.1.3 // indirect
+	github.com/linyows/go-onigmo v0.1.0


binding for onigmo

erizocosmico · 2019-02-19T08:46:52Z

internal/regex/regex_test.go

+
+		if n == "hyperscan" {
+			return "hyperscan"
+		}


all these lines can be transformed into:

switch n { case "onigmo", "oniguruma", "hyperscan": return n }

erizocosmico · 2019-02-19T08:47:24Z

internal/regex/regex_onigmo.go

+	if err != nil {
+		return nil, err
+	}
+	runtime.SetFinalizer(re, (*onigmo.Regexp).Free)


Finalizers are not guaranteed to be called. I don't think it's a proper mechanism to free the regexps.

The safest refactoring (IMO) is to extend our matcher interface by adding close/free function. Or maybe we can sacrifice some performance and have only one Match function which will be registered and do all - compile/match/free.
WDYT?

I think we can check in the usage place if the matcher implements io.Closer and then implement Close() error method only in the matchers that need some closing.

That way, current matchers do not need any refactoring and nothing needs to change. Then, in the place we use the matcher we add the io.Closer spec and call it if needed.

erizocosmico · 2019-02-19T08:47:39Z

internal/regex/regex_hyperscan.go

+	if err != nil {
+		return nil, err
+	}
+	runtime.SetFinalizer(db, (hyperscan.Database).Close)


same with this

erizocosmico · 2019-02-19T08:48:42Z

.travis.yml

+  - cd onigmo-6.2.0
+  - ./configure
+  - make
+  - sudo make install


We should also update the docs about how to install onigmo. Isn't there a more user-friendly way of installing it?

Frankly I could not find any convenient way (like apt-get or brew), so I compiled from source. The good think - there is no dependencies, so it compiles smoothly.

I've updated a README, as well.

kuba-- · 2019-02-22T13:35:06Z

@ajnavarro, @erizocosmico - guys take a look into the next iteration.
After y'da's discussion about regex engine(s), we made a decision to make oniguruma a default one (if you want go's regex, you need to compile mysql with --tags go).
Apart from that, I've added a Disposer (next to Matcher) to our API. it's mainly for C-libs.
Basically Disposer will be called if we don't want to cache compiled matchers.
PTAL

erizocosmico · 2019-02-22T13:36:39Z

Didn't oniguruma use to segfault/panic in certain times? That's why we removed it. Was that fixed? If not, why are we adding this back again?

kuba-- · 2019-02-22T13:36:53Z

The next step is to integrate it in gitbase. On gitbase only oniguruma will be available. We can think about GOREGEXP queries (where we'll use go std. lib), but it will require changes in vitess parser.

kuba-- · 2019-02-22T13:38:19Z

Regarding #618 (comment)
In my opinion it was fixed after I added a Pool but maybe @jfontan knows more details.

ajnavarro · 2019-02-22T14:28:01Z

@kuba-- could you change the PR title please? It's a little bit misleading now.

ajnavarro · 2019-02-22T14:29:44Z

README.md

+make oniguruma # on linux you may need sudo
+```
+
+If you want to use regex engine from go's standard library, you have to compile **go-mysql-server** with `--tags go` argument.


shall we change the tag to something more specific, like golang_regex_engine or something like that?

there are a lot of possibilities that another library is using the same go tag

if this is a risk, so lets prefix it with mysql_

In my opinion, mysql_go is too broad, but maybe is the go way ;)

ajnavarro · 2019-02-22T14:31:05Z

README.md

+```
+
+If you want to use regex engine from go's standard library, you have to compile **go-mysql-server** with `--tags go` argument.
+Besides standard go's library and [oniguruma](github.com/kkos/oniguruma) the **go-mysql-server** also supports multiple engines originaly implemented in C.


I would remove any other regex engine and focus on only one.

jfontan · 2019-02-22T16:09:34Z

As @kuba says in #618 (comment) I believe it was fixed with the regexp Pool.

kuba-- · 2019-02-22T21:41:12Z

@ajnavarro the build finally passed
Shall I change go tag to something like mysql-go-regex?
Also question - totally remove other engines, or rename tags to follow the same pattern:
mysql-onigmo-regex
mysql-hyperscan-regex
?

ajnavarro · 2019-02-25T08:53:08Z

I would say to totally remove old engines. Fewer engines, less code to maintain. If we need them in the future we can go to the commit history and get them.

mysql-go-regex sounds good.

Signed-off-by: kuba-- <kuba@sourced.tech>

kuba-- · 2019-02-25T13:16:40Z

@ajnavarro - PTAL. The new tag is mysql_go_regex.

Signed-off-by: kuba-- <kuba@sourced.tech>

kuba-- · 2019-02-25T15:46:00Z

Updated after go mod tidy

ajnavarro

LGTM except for the go.mod file, that should be updated.

kuba-- · 2019-02-25T15:48:15Z

@kuba-- - it was just updated (go mod tidy) - PTAL

erizocosmico approved these changes Feb 14, 2019

View reviewed changes

kuba-- changed the title ~~Integrate hyperscan as a new regex engine~~ Integrate hyperscan and onigmo as a new regex engines Feb 15, 2019

kuba-- commented Feb 15, 2019

View reviewed changes

kuba-- requested a review from ajnavarro February 15, 2019 00:57

kuba-- added the enhancement New feature or request label Feb 15, 2019

erizocosmico suggested changes Feb 19, 2019

View reviewed changes

erizocosmico approved these changes Feb 22, 2019

View reviewed changes

ajnavarro reviewed Feb 22, 2019

View reviewed changes

kuba-- changed the title ~~Integrate hyperscan and onigmo as a new regex engines~~ Make the oniguruma a default regex engine Feb 22, 2019

kuba-- changed the title ~~Make the oniguruma a default regex engine~~ Make oniguruma a default regex engine Feb 22, 2019

ajnavarro reviewed Feb 22, 2019

View reviewed changes

Make oniguruma as a default regex engine.

517a2d9

Signed-off-by: kuba-- <kuba@sourced.tech>

go mod tidy

cad45bc

Signed-off-by: kuba-- <kuba@sourced.tech>

ajnavarro approved these changes Feb 25, 2019

View reviewed changes

ajnavarro merged commit bdd9e0c into src-d:master Feb 25, 2019

kuba-- deleted the perf-615/regex branch February 25, 2019 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make oniguruma a default regex engine #618

Make oniguruma a default regex engine #618

kuba-- commented Feb 13, 2019

kuba-- Feb 15, 2019

kuba-- Feb 15, 2019

ajnavarro Feb 25, 2019

kuba-- Feb 25, 2019

kuba-- Feb 15, 2019

erizocosmico Feb 19, 2019

erizocosmico Feb 19, 2019

kuba-- Feb 19, 2019

erizocosmico Feb 19, 2019

erizocosmico Feb 19, 2019

erizocosmico Feb 19, 2019

kuba-- Feb 19, 2019

kuba-- Feb 19, 2019

kuba-- commented Feb 22, 2019

erizocosmico commented Feb 22, 2019

kuba-- commented Feb 22, 2019

kuba-- commented Feb 22, 2019

ajnavarro commented Feb 22, 2019

ajnavarro Feb 22, 2019

ajnavarro Feb 22, 2019

kuba-- Feb 22, 2019

ajnavarro Feb 22, 2019

ajnavarro Feb 22, 2019

jfontan commented Feb 22, 2019

kuba-- commented Feb 22, 2019

ajnavarro commented Feb 25, 2019

kuba-- commented Feb 25, 2019

kuba-- commented Feb 25, 2019

ajnavarro left a comment

kuba-- commented Feb 25, 2019

Make oniguruma a default regex engine #618

Make oniguruma a default regex engine #618

Conversation

kuba-- commented Feb 13, 2019

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

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

kuba-- commented Feb 22, 2019

erizocosmico commented Feb 22, 2019

kuba-- commented Feb 22, 2019

kuba-- commented Feb 22, 2019

ajnavarro commented Feb 22, 2019

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

jfontan commented Feb 22, 2019

kuba-- commented Feb 22, 2019

ajnavarro commented Feb 25, 2019

kuba-- commented Feb 25, 2019

kuba-- commented Feb 25, 2019

ajnavarro left a comment

Choose a reason for hiding this comment

kuba-- commented Feb 25, 2019