Skip to content

disable kickstart in ctfe to workaround Issue 16626#4995

Merged
MartinNowak merged 1 commit intodlang:masterfrom
MartinNowak:fix16626
Dec 26, 2016
Merged

disable kickstart in ctfe to workaround Issue 16626#4995
MartinNowak merged 1 commit intodlang:masterfrom
MartinNowak:fix16626

Conversation

@MartinNowak
Copy link
Member

- consumes too much memory, introduced by
  e98fa4a (dlang#4286)
@dlang-bot
Copy link
Contributor

dlang-bot commented Dec 24, 2016

Fix Bugzilla Description
16626 [Reg 2.073] extreme CTFE memory usage with compile time regex

@wilzbach
Copy link
Contributor

Don't worry about the CodeCov messages. Due to Issue 16397 the numbers for running the entire test suite are wrong and we need to run the coverage analysis per module.

@wilzbach wilzbach added this to the 2.073.0-b0 milestone Dec 25, 2016
@MartinNowak
Copy link
Member Author

Going to merge this so we can reenable the Higgs tests. It was fairly similar to @DmitryOlshansky's initial fix attempt and disabling the kickstarter should only make the Regex slower, but not break it.

@MartinNowak MartinNowak merged commit 9cbc862 into dlang:master Dec 26, 2016
@MartinNowak MartinNowak deleted the fix16626 branch December 26, 2016 23:33
@DmitryOlshansky
Copy link
Member

Thanks!
Sadly this means ctRegex is slower then normal regex in most cases. Anyhow until compiler is better at CTFE we are out of options.

@nordlow
Copy link
Contributor

nordlow commented Dec 27, 2016

We just need to wait for @UplinkCoder to finish newCTFE. 😊

@nordlow
Copy link
Contributor

nordlow commented Dec 27, 2016

@JackStouffer
Copy link
Contributor

JackStouffer commented Feb 9, 2017

@UplinkCoder
Copy link
Member

UplinkCoder commented Feb 9, 2017

@JackStouffer I can indeed reproduce this.
We cannot re-enable this as long as it crashes the compiler.

@JackStouffer
Copy link
Contributor

But we can temporarily revert to the way things were done in 2.072.2. Which we should do ASAP, as CTFE regex is one of the poster-children for D. Hell, Andrei makes a point to mention how fast it's supposed to be every talk he does on D.

@DmitryOlshansky
Copy link
Member

But we can temporarily revert to the way things were done in 2.072.2. Which we should do ASAP, as CTFE regex is one of the poster-children for D.

For the record - it works, just consumes more memory then typical tester boxes seem to have.
About the revert - the PR to get new engine was a massive and long overdue rework of std.regex internals. I was waiting for years to enable const Regex! Postponing it for another half a year (or more) is not something I'm looking forward to.

I'm being held back by the quality of CTFE ever since 2011. I'm afraid that without a NAGGING ISSUE that we MUST improve the state of CTFE nothing will be done in our time. Humiliation could be a decent motivator.

Hell, Andrei makes a point to mention how fast it's supposed to be every talk he does on D.

In fact in following a discussion of std.regex state with Andrei I prepared a next patch that brings ctRegex to the new level of performance. I even tested it against top competitors (with JITs and whatnot) and verified that we indeed can be the fastest on Earth. Guess what? Even more memory hungry due to CTFE.

TL;DR: We will get nothing while CTFE is a piece of junk. I'm accepting this regression and do not think that freezing std.regex development until CTFE is improved is a good idea.

@UplinkCoder
Copy link
Member

@DmitryOlshansky
I am improving CTFE as fast as I can, but it will take a few months until it can support std.regex.
If your test-boxes cannot compile the projects they are supposed to test this is a problem.

@JackStouffer
Copy link
Contributor

JackStouffer commented Feb 10, 2017

@DmitryOlshansky Then develop it in a separate branch and merge it into master when the new CTFE engine is done. In the mean time we can revert the whole regex folder to the v2.072.2 branch and performance and our image are both saved.

Intentionally decreasing performance until a nebulous future date when we have a better CTFE engine is untenable for a standard library feature which many depend on to be fast.

@DmitryOlshansky
Copy link
Member

@UplinkCoder Thanks for stepping up and doing all the hard work! A few months sounds way more inspiring the yet another year or so.

@JackStouffer You are probably right, I just have to muster my patience meanwhile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants