https://d.puremagic.com/issues/show_bug.cgi?id=11783

           Summary: Make std.datetime unittesting faster
           Product: D
           Version: D2
          Platform: All
        OS/Version: All
            Status: NEW
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: nobody@puremagic.com
        ReportedBy: andrei@erdani.com


--- Comment #0 from Andrei Alexandrescu <andrei@erdani.com> 2013-12-19 20:27:49 PST ---
std.datetime is extremely slow to unittest even on very fast machines. It's more than one order of magnitude slower to unittest than any other Phobos module. This makes even parallel unittest builds unduly slow. There is no justification - testing time should be slashed by 10x.

-- 
Configure issuemail: https://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

https://d.puremagic.com/issues/show_bug.cgi?id=11783


yebblies <yebblies@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |yebblies@gmail.com


--- Comment #1 from yebblies <yebblies@gmail.com> 2013-12-20 16:34:07 EST ---
We can get infinityx speedup by simply deleting _all_ the tests in std.datetime!

-- 
Configure issuemail: https://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

https://d.puremagic.com/issues/show_bug.cgi?id=11783



--- Comment #2 from Andrei Alexandrescu <andrei@erdani.com> 2013-12-21 13:42:52 PST ---
(In reply to comment #1)
> We can get infinityx speedup by simply deleting _all_ the tests in std.datetime!

I get the sarcasm but it's misapplied. There's no justification that testing code for dates and times takes more than 10 seconds on a $10K blade. std.datetime is the holder of quite a few dubious records, and it's past time we fix that.

-- 
Configure issuemail: https://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

https://d.puremagic.com/issues/show_bug.cgi?id=11783


Jonathan M Davis <jmdavisProg@gmx.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jmdavisProg@gmx.com


--- Comment #3 from Jonathan M Davis <jmdavisProg@gmx.com> 2013-12-21 21:25:43 PST ---
I'm divided on this sort of thing, because I tend to think that other modules actually test too little rather than std.datetime testing too much as far as properly testing goes (I really do think that a lot of our testing is too sparse - particularly when it comes to range based functions). However, std.datetime's tests do need to be refactored, and the fact they take as long as they do does stand out and is problematic. Thorough testing catches and prevents bugs, but tests that take a long time get in the way of development. One of the factors involved is the fact that D's exceptions are really slow - issue# 9584 - which affects any test verifying that a function throws when it's supposed to, and I expect that some refactoring of the order of some of what the tests are doing would make them faster, but obviously the number of tests involved makes it take longer regardless of what they're doing.

One possibility is to section of some of the tests so that they only run when a particular version is set so that they don't run normally but can be turned on when more thorough testing is warranted. Or I could move some of them into a separate test suite (though that would mean that only I would have them, which wouldn't be good). Regardless, there are ways to decrease how many tests are run during the normal Phobos build without losing the tests. And any speed-ups in the tests themselves would obviously also help.

Refactoring std.datetime's unit tests have been on my todo list for after I finish splitting std.datetime (which is mostly done), but I've been completely bogged down at work for the past few months, so I haven't been making any real progress on anything D related lately (or anything else outside of work for that matter). However, I expect to be able to get back to this sort of thing in January or February.

-- 
Configure issuemail: https://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

https://d.puremagic.com/issues/show_bug.cgi?id=11783



--- Comment #4 from Andrei Alexandrescu <andrei@erdani.com> 2013-12-22 00:10:29 PST ---
(In reply to comment #3)
> I'm divided on this sort of thing, because I tend to think that other modules actually test too little rather than std.datetime testing too much as far as properly testing goes (I really do think that a lot of our testing is too sparse - particularly when it comes to range based functions).

I agree Phobos could use more and better unittests. That's not an argument in support for the state of affairs with std.datetime's unittests.

On a top-of-the-line machine, testing std.datetime takes 38 seconds to run (in release mode). On the same machine testing std.algorithm takes 0.15 seconds and testing std.range takes 0.1 seconds. I agree both std.algorithm and std.range could use more unittesting, but probably not quite 300x more.

> However,
> std.datetime's tests do need to be refactored, and the fact they take as long
> as they do does stand out and is problematic.

Great, thanks.

> One of the factors involved is the fact that D's exceptions are really slow - issue# 9584 - which affects any test verifying that a function throws when it's supposed to, and I expect that some refactoring of the order of some of what the tests are doing would make them faster, but obviously the number of tests involved makes it take longer regardless of what they're doing.

I frankly don't think that's the high level problem in this case.

> One possibility is to section of some of the tests so that they only run when a particular version is set so that they don't run normally but can be turned on when more thorough testing is warranted. Or I could move some of them into a separate test suite (though that would mean that only I would have them, which wouldn't be good). Regardless, there are ways to decrease how many tests are run during the normal Phobos build without losing the tests. And any speed-ups in the tests themselves would obviously also help.

I don't think this is a good solution. I am confident that 4000 lines of code can be tested thoroughly in a reasonable time budget.

> Refactoring std.datetime's unit tests have been on my todo list for after I finish splitting std.datetime (which is mostly done), but I've been completely bogged down at work for the past few months, so I haven't been making any real progress on anything D related lately (or anything else outside of work for that matter). However, I expect to be able to get back to this sort of thing in January or February.

Good luck with work, of course it takes priority. I will try to make time for this myself, the situation has gotten quite bad.

-- 
Configure issuemail: https://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

https://d.puremagic.com/issues/show_bug.cgi?id=11783



--- Comment #5 from Jonathan M Davis <jmdavisProg@gmx.com> 2013-12-22 01:12:36 PST ---
It wouldn't surprise me if _assertPred adds quite a bit to it, because of all of the template instantiations that get generated for it. That might be part of why the tests take so long. Some of the tests were refactored to use plain assertions after assertPred failed to get through the review process, but a good chunk of them still use _assertPred, so if those template instantions incur enough of a performance hit, then removing those may help cut down on the compilation time for the tests (AFAIK, the time for the tests includes both the compilation time and the running time, but I don't know - if it's just the running time, then this probably isn't a factor).

I do know for a fact though that the assertThrown tests are very expensive and cut down on them at one point because of the ridiculously low performance of D exceptions. I really don't like cutting out too many of them, as I think that the failure cases should be well tested as well, but it's possible that those tests alone are a large portion of the running time. That's why I really wish that issue# 9584 would get some attention. Exception performance is one place where Java completely blows us out of the water, and while it might not matter a lot for production code (as exceptions should be relatively rare there), it's very detrimental to unit tests which test failure cases.

-- 
Configure issuemail: https://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

https://d.puremagic.com/issues/show_bug.cgi?id=11783



--- Comment #6 from Andrei Alexandrescu <andrei@erdani.com> 2013-12-22 02:21:52 PST ---
(In reply to comment #5)
> It wouldn't surprise me if _assertPred adds quite a bit to it, because of all of the template instantiations that get generated for it. That might be part of why the tests take so long. Some of the tests were refactored to use plain assertions after assertPred failed to get through the review process, but a good chunk of them still use _assertPred, so if those template instantions incur enough of a performance hit, then removing those may help cut down on the compilation time for the tests (AFAIK, the time for the tests includes both the compilation time and the running time, but I don't know - if it's just the running time, then this probably isn't a factor).
> 
> I do know for a fact though that the assertThrown tests are very expensive and cut down on them at one point because of the ridiculously low performance of D exceptions. I really don't like cutting out too many of them, as I think that the failure cases should be well tested as well, but it's possible that those tests alone are a large portion of the running time. That's why I really wish that issue# 9584 would get some attention. Exception performance is one place where Java completely blows us out of the water, and while it might not matter a lot for production code (as exceptions should be relatively rare there), it's very detrimental to unit tests which test failure cases.

I understand you consider the speed of throwing exceptions an important factor here. I'm sorry I haven't been equally clear about expressing my point. It is my opinion that the test taking long is a systemic problem with the unittests in std.datetime, not with the speed of D exceptions. Improving the latter is of course desirable but that is largely independent of the issue at hand.

Thirty-eight seconds is an eternity in CPU time. One would need to exercise a large amount of abuse on the CPU to even get in that vicinity - especially if files and networks are not being used. Conversely, it should be fairly easy to eliminate that abuse. A quick browse shows things like

        foreach(year; chain(testYearsBC, testYearsAD))
        {
            foreach(md; testMonthDays)
            {
                foreach(tod; testTODs)
                {
                    auto dt = DateTime(Date(year, md.month, md.day), tod);

                    foreach(tz; testTZs)
                    {
                        foreach(fs; testFracSecs)
                            test(SysTime(dt, fs, tz), year);
                    }
                }
            }
        }

I have no idea whether that's a major contributor to the total runtime, but on the face of it five nested loops are what they are and most likely are testing redundantly an awfully small area of code.

-- 
Configure issuemail: https://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

https://d.puremagic.com/issues/show_bug.cgi?id=11783



--- Comment #7 from Jonathan M Davis <jmdavisProg@gmx.com> 2013-12-22 03:10:47 PST ---
There are lots of corner cases in date-time calculations, so without being thorough, bugs can easily go unnoticed where the code works in almost all cases but not with a particular date or time. So, the tests try and be very thorough. And yes, that cost adds up. But I really don't see any way around it without risking bugs. The odds of such bugs are much lower now that the code has already been written and tested, but any refactoring that occurs would risk reintroducing bugs, and without thorough tests, the odds are much higher that a corner case won't be tested well enough, and bugs will creep in. For instance, at one point in the development process of std.datetime, there was a bug relating to years in B.C. that ended with 99, and it just so happened that I hadn't tested such a year, so I hadn't caught the bug. It's frustratingly easy to have a particular date that doesn't work when almost every other date does, so while I agree that the running time for std.datetime's unit tests needs to be reduced, I'm very leery about reducing them and would be inclined to find ways to make them more efficient without reducing how much is actually being tested (or worst case, make it so that some of the tests are disabled normally, though I don't particularly like that idea). I'm sure that improvements could be made in that area, but I haven't had time to spend on it any time recently.

The problem with the exception tests is that they take far, far more time than the other tests, so if you have many of them, the time balloons quite quickly, whereas you have to add a lot more other tests to get the same kind of increase in execution time. So, in order to keep their cost low, you're forced to do far fewer tests for the failure cases, which is very frustrating. I don't know what percentage of the execution time the exception tests currently are and am certainly not trying to claim that the std.datetime unit tests take as long as they do simply because exceptions are too slow in D, but due to how expensive they are, they do make the situation worse, and it's quite possible that a poorly chosen loop which had assertThrown is adding several seconds to the execution time. It could still very well be the case that almost all of the time is spent on other tests. I pointed it out because it is a known area which can cause an inordinately large increase in the excecution time and thus could be low hanging fruit, not because I was trying to claim that std.datetime's tests are slow simply due to bug# 9584.

And worst case, if std.datetime's unit tests are too big a problem for you, you can comment out a bunch of them or remove the line which sets the version testStdDateTime, which would disable many of the tests (many of them are still enabled via that since I haven't finished removing it yet). It probably won't affect much until someone actually makes changes to std.datetime, which doesn't happen very often. So, while I think that that's a bad strategy long term, it could be a quick way to reduce how much of a problem they're causing until I have the chance to refactor them in a month or two.

-- 
Configure issuemail: https://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

https://d.puremagic.com/issues/show_bug.cgi?id=11783



--- Comment #8 from Andrei Alexandrescu <andrei@erdani.com> 2013-12-22 03:56:22 PST ---
(In reply to comment #7)
> There are lots of corner cases in date-time calculations, so without being thorough, bugs can easily go unnoticed where the code works in almost all cases but not with a particular date or time. So, the tests try and be very thorough. And yes, that cost adds up. But I really don't see any way around it without risking bugs. The odds of such bugs are much lower now that the code has already been written and tested, but any refactoring that occurs would risk reintroducing bugs, and without thorough tests, the odds are much higher that a corner case won't be tested well enough, and bugs will creep in. For instance, at one point in the development process of std.datetime, there was a bug relating to years in B.C. that ended with 99, and it just so happened that I hadn't tested such a year, so I hadn't caught the bug. It's frustratingly easy to have a particular date that doesn't work when almost every other date does, so while I agree that the running time for std.datetime's unit tests needs to be reduced, I'm very leery about reducing them and would be inclined to find ways to make them more efficient without reducing how much is actually being tested (or worst case, make it so that some of the tests are disabled normally, though I don't particularly like that idea). I'm sure that improvements could be made in that area, but I haven't had time to spend on it any time recently.
> 
> The problem with the exception tests is that they take far, far more time than the other tests, so if you have many of them, the time balloons quite quickly, whereas you have to add a lot more other tests to get the same kind of increase in execution time. So, in order to keep their cost low, you're forced to do far fewer tests for the failure cases, which is very frustrating. I don't know what percentage of the execution time the exception tests currently are and am certainly not trying to claim that the std.datetime unit tests take as long as they do simply because exceptions are too slow in D, but due to how expensive they are, they do make the situation worse, and it's quite possible that a poorly chosen loop which had assertThrown is adding several seconds to the execution time. It could still very well be the case that almost all of the time is spent on other tests. I pointed it out because it is a known area which can cause an inordinately large increase in the excecution time and thus could be low hanging fruit, not because I was trying to claim that std.datetime's tests are slow simply due to bug# 9584.
> 
> And worst case, if std.datetime's unit tests are too big a problem for you, you can comment out a bunch of them or remove the line which sets the version testStdDateTime, which would disable many of the tests (many of them are still enabled via that since I haven't finished removing it yet). It probably won't affect much until someone actually makes changes to std.datetime, which doesn't happen very often. So, while I think that that's a bad strategy long term, it could be a quick way to reduce how much of a problem they're causing until I have the chance to refactor them in a month or two.

Jonathan, I seem to have difficulty getting my point across and the longer the responses become the larger the disconnect.

Your three long paragraphs seem to convey the following points, one per paragraph:

1. Dates and times are special in ways that make them impossible to test in less than an ungodly long amount of time.

2. Again, it's the exceptions that make things so slow; there's no way to improve timing without cutting down on essential tests.

3. It seems I am the only person who has a problem with a 38 seconds test on modest functionality. Furthermore, the engineering solution proposed is, again, to remove some tests with the note that that actually is a bad thing.

They fail to discuss or even so much acknowledge the simple matter that std.datetime has its own grave issues. I very much disagree with this view and the general attitude that the problem is in various other places than simply inside std.datetime and the way its unittests are engineered. If one were to take at face value these increasingly long replies, the only logical conclusion one could draw is that std.datetime is perfectly engineered and the only way to make its unittests any faster is to improve the compiler implementation.

-- 
Configure issuemail: https://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------

https://d.puremagic.com/issues/show_bug.cgi?id=11783



--- Comment #9 from Jonathan M Davis <jmdavisProg@gmx.com> 2013-12-22 04:53:05 PST ---
> Jonathan, I seem to have difficulty getting my point across and the longer the
responses become the larger the disconnect.

Yes. It does look that way. Maybe this would go better if I were better at being concise.

> 1. Dates and times are special in ways that make them impossible to test in less than an ungodly long amount of time.

Not at all. The issue is that the tests need to be very thorough and that that can be expensive timewise. That does not mean at all that they have to take as long as they currently do. My objection is only to reducing how thorough the tests are. I'm sure that it's possible to refactor them so that they take less time without reducing how much they test.

2. Again, it's the exceptions that make things so slow; there's no way to
> improve timing without cutting down on essential tests.

The exceptions _are_ slow and contribute to the problem, but I'm sure that they're not the whole the problem. I brought them up as a likely place where small changes could provide large gains. I did not mean to try and claim that they were the whole problem, but maybe my frustration with them being slow made me come across that way. Regardless of how much the exceptions cost, the other tests do definitely need to be optimized.

I guess that I never should have brought up the exceptions. Along with bringing up _assertPred, I was trying to give suggestions as to where some likely culprits for the slow tests might be beyond simply the number of tests. I was not trying to give excuses for the quality of my code or its tests.

> 3. It seems I am the only person who has a problem with a 38 seconds test on modest functionality. Furthermore, the engineering solution proposed is, again, to remove some tests with the note that that actually is a bad thing.

I completely agree that 38 seconds is too long. That needs to be significantly reduced. However, what I don't want to do is reduce it by simply reducing the number of tests. Experience has shown me (and other developers who I've talked to agree with me - e.g. Steven Schveighoffer who did the Tango time stuff years ago) that date/time-related tests need to be thorough because of all of the corner cases, many of which are hard to know because they depend on quirks of the calculations being done. Maybe the number of tests will have to be reduced in order to get the time down, but if I can, I'd very much prefer to get the time down by optimizing the tests, which I believe can be done.

> They fail to discuss or even so much acknowledge the simple matter that
> std.datetime has its own grave issues. I very much disagree with this view and
> the general attitude that the problem is in various other places than simply
> inside std.datetime and the way its unittests are engineered. If one were to
> take at face value these increasingly long replies, the only logical
> conclusion one could draw is that std.datetime is perfectly engineered and the > only way to make its unittests any faster is to improve the compiler
> implementation.

I do not claim for a second that std.datetime is perfectly engineered, and I definitely don't claim that the tests are perfectly engineered. They were written to be thorough, not fast. And clearly they need to be refactored so that they're much faster than they are. I just don't want to get in a situation where what's being tested is reduced in order to get the test times down, as I believe that that will significantly increase the risk of bugs whenever the code being tested is refactored. If anything, I'm very concerned about having thorough testing because I feel that my code quality would be at much greater risk without it.

Maybe I'm too touchy on this subject and come across badly, for which I'm sorry. But I believe very strongly that code in general (not just std.datetime) needs to have very thorough testing, and so I tend to object to attempts to reduce the number of tests of any code. A natural side effect of such testing is that it can get too expensive, which requires better optimizing it and being more careful with how you go about testing in order to avoid having the tests' running time increase too much. And clearly, I have not managed to do that with std.datetime, and that needs to be fixed.

I've been meaning to refactor std.datetime's unit tests but have not made it a high enough priority. Clearly, that needs to be among my highest priorities with regards to the work I do on D.

-- 
Configure issuemail: https://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------