Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Issue 18114 - regex performance regression #5981

Merged
merged 3 commits into from
Feb 22, 2018

Conversation

MartinNowak
Copy link
Member

@MartinNowak MartinNowak commented Jan 2, 2018

  • reduce copying of fat structs
  • optimize layouts and opAssign of Captures

WIP: still need to figure out how to best handle ref passing of ctRegex

@dlang-bot
Copy link
Contributor

dlang-bot commented Jan 2, 2018

Thanks for your pull request, @MartinNowak!

Bugzilla references

Auto-close Bugzilla Severity Description
18114 regression [Reg 2.078] regex performance regression

@MartinNowak MartinNowak changed the title fix Issue 18114 - regex performance regression [WIP] fix Issue 18114 - regex performance regression Jan 2, 2018
@dlang-bot dlang-bot added the WIP Work In Progress - not ready for review or pulling label Jan 2, 2018
@MartinNowak
Copy link
Member Author

Somewhat tricky, but would be best if we could deprecate usage of ctRegexs as mutable Regex, instead making "logically const" copies.
https://ci.dlang.io/blue/organizations/jenkins/dlang-org%2Fphobos/detail/PR-5981/1/pipeline/184#step-334-log-10

Copy link
Member

@DmitryOlshansky DmitryOlshansky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

And thanks for figuring it out

// allow code that expects mutable Regex to still work
// we stay "logically const"
@trusted @property auto getRe() const { return cast() staticRe; }
@property ref getRe() const { return staticRe; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall some code that broke because of this change on Jenkins auto-tester.

Dunno if we should could overload both by ref and bu value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the problem would replacing ref with auto ref fix it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall some code that broke because of this change on Jenkins auto-tester.

Yeah. Higgs fails:

parser/lexer.d(548): Error: none of the overloads of 'match' are callable using argument types (Wrapper), candidates are:
parser/lexer.d(335):        parser.lexer.StrStream.match(wstring str)
parser/lexer.d(351):        parser.lexer.StrStream.match(Regex!wchar re)
parser/lexer.d(664): Error: none of the overloads of 'match' are callable using argument types (Wrapper), candidates are:
parser/lexer.d(335):        parser.lexer.StrStream.match(wstring str)
parser/lexer.d(351):        parser.lexer.StrStream.match(Regex!wchar re)
parser/lexer.d(687): Error: none of the overloads of 'match' are callable using argument types (Wrapper), candidates are:
parser/lexer.d(335):        parser.lexer.StrStream.match(wstring str)
parser/lexer.d(351):        parser.lexer.StrStream.match(Regex!wchar re)
parser/lexer.d(699): Error: none of the overloads of 'match' are callable using argument types (Wrapper), candidates are:
parser/lexer.d(335):        parser.lexer.StrStream.match(wstring str)
parser/lexer.d(351):        parser.lexer.StrStream.match(Regex!wchar re)
makefile:77: recipe for target 'test' failed

https://ci.dlang.io/blue/organizations/jenkins/dlang-org%2Fphobos/detail/PR-5981/1/pipeline

- reduce copying of fat structs
- optimize layouts and opAssign of Captures
@MartinNowak
Copy link
Member Author

MartinNowak commented Feb 22, 2018

Rebased and ready to be pulled.
5 ❤️s if someone figures out how to deprecate usage of ctRegex as mutable regex. Casting immutable data (stored in const object file space) to a mutable ref is an ugly workaround.

- needed for compatibility with existing ctRegex users (e.g. Higgs)
- cast reference instead of value to avoid copying
@MartinNowak MartinNowak changed the title [WIP] fix Issue 18114 - regex performance regression fix Issue 18114 - regex performance regression Feb 22, 2018
@MartinNowak MartinNowak removed the WIP Work In Progress - not ready for review or pulling label Feb 22, 2018
@MartinNowak
Copy link
Member Author

Auto-merge toggled on

@dlang-bot dlang-bot merged commit 460693c into dlang:stable Feb 22, 2018
@MartinNowak MartinNowak deleted the issue18114 branch February 23, 2018 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants