Create pkg/regexp to better handle init regex #1465
Conversation
|
looks great to me. Should potentially be somewhere not in containers/storage though... Maybe containers/common? |
|
LGTM, I like the idea. The only risk is that the regex using the new package can panic at any point during the program execution (potentially causing db corruptions), while before we'd catch the error immediately; but I don't see an immediate way to address it. Maybe an env variable that causes the regexp to be compiled immediately and enable it in the CI? |
|
The reonce package has:
If we did this, the CI could enable this. |
|
@alexlarsson Containers/storage is vendored into containers/common, not the other way around This is how packages are vendored Each package vendors the packages above. containers/storage |
|
@alexlarsson @giuseppe added regex_precompile build tag |
pkg/regexp/regexp.go
Outdated
|
|
||
| // Regexp is a wrapper struct used for wrapping MustCompile regex expressions | ||
| // used init. Using this stucture should help speed up startup time of apps | ||
| // that want to initialize their regex at init time. |
There was a problem hiding this comment.
| // that want to initialize their regex at init time. | |
| // that do not want to initialize their regex at init time. |
|
The goal of the tag is to blow up if the REGEX is bad, during testing versus production, not likely, but might as well provide functionality. |
pkg/regexp/regexp_dontprecompile.go
Outdated
|
|
||
| package regexp | ||
|
|
||
| var precompile = false |
There was a problem hiding this comment.
Yes (in both cases); this decision should have zero run-time cost.
| @@ -0,0 +1,210 @@ | |||
| package regexp | |||
There was a problem hiding this comment.
I’m not sure about using the same name as the standard library package, the two are not interoperable in any sense. OTOH there’s a certain elegance to this.
There was a problem hiding this comment.
Having the same package name does make it tricky to use this and call regular regexp global functions from the same file (you'd have to rename one of them), so i agree with this.
There was a problem hiding this comment.
OTOH that could be useful as a speed bump to make the user consider whether that call should use the delayed package as well.
There was a problem hiding this comment.
Well, for dynaminc use (i.e. create a regexp in a function) you never want the lazy stuff.
| } | ||
|
|
||
| func (re *Regexp) compile() { | ||
| re.once.Do(func() { |
There was a problem hiding this comment.
Don’t do this if precompile?
On the one hand, the current version makes run-time profiling of the precompile version representative of the other one.
On the other hand, making compilation conditional here would allow an easy experiment to see if precompilation, in total, pays off in a particular situation.
There was a problem hiding this comment.
Ok, so you want the re.once.Do command not to be executed.
There was a problem hiding this comment.
My very weak preference would be to not execute it, but I’m completely fine with either one.
rhatdan
force-pushed
the
regexp
branch
2 times, most recently
from
58c9b92
to
776147b
Compare
The new Reexp structure, will handle the initialization of regexp and then will do the MustCompile within a sync.Once field. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
The new Reexp structure, will handle the initialization of regexp and then will do the MustCompile within a sync.Once field.
Signed-off-by: Daniel J Walsh dwalsh@redhat.com