Dramatically faster to import std.datetime (and anything under it) #5948

andralex · 2017-12-19T22:20:05Z

This has been an iterative process of pushing imports from the top level into the places where imports are being used. That helped not only clarifying dependencies, but also accelerates import time by about 3x. Further improvements are possible.

dlang-bot · 2017-12-19T22:20:07Z

Thanks for your pull request, @andralex!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

quickfur · 2017-12-19T23:29:44Z

Very nice! Hooray for local imports. They are an overall win.

You could also reference https://issues.dlang.org/show_bug.cgi?id=13253 in one of your commits; it's a useful way to keep track of improvements that we've made in turning more imports local.

wilzbach

Windows doesn't like this yet:

std\datetime\systime.d(9304): Error: undefined identifier `msecs`
std\datetime\timezone.d(3038): Error: undefined identifier `DateTimeException`
std\datetime\timezone.d(3081): Error: undefined identifier `DateTime`
std\datetime\timezone.d(3086): Error: undefined identifier `Month`
std\datetime\timezone.d(3092): Error: undefined identifier `Month`
std\datetime\timezone.d(3101): Error: undefined identifier `Month`
std\datetime\timezone.d(3106): Error: undefined identifier `Month`
std\datetime\timezone.d(3125): Error: undefined identifier `DateTime`
std\datetime\timezone.d(3158): Error: template instance convert!("minutes", "hnsecs") template 'convert' is not defined
std\datetime\timezone.d(3160): Error: template instance convert!("minutes", "hnsecs") template 'convert' is not defined
std\datetime\timezone.d(3170): Error: undefined identifier `DateTime`
std\datetime\timezone.d(3234): Error: undefined identifier `DateTime`
std\datetime\timezone.d(3256): Error: template instance convert!("minutes", "hnsecs") template 'convert' is not defined
std\datetime\timezone.d(3262): Error: template instance convert!("minutes", "hnsecs") template 'convert' is not defined

wilzbach · 2017-12-20T07:55:52Z

std/datetime/timezone.d

@@ -1385,6 +1400,9 @@ public:
    this(Duration utcOffset, string stdName = "") @safe immutable pure
    {
        // FIXME This probably needs to be changed to something like (-12 - 13).


Maybe the imports should be above this comment?

wilzbach · 2017-12-20T10:28:01Z

BTW during my work on this, I noticed a couple of things:

We shouldn't be doing this by hand. At least the conversion from full imports to selective imports could be automated
If we would have lazy selective imports, then all this work isn't needed and from a naive point of view, this shouldn't be too hard? The compiler knows where he can find a symbol and only needs to so if he really needs that symbol resolved...
There's currently no way to make imports in aggregated types lazy (except for using Voldemort types, but that would be a breaking change for public symbols). Example:

struct Foo
{
    import std.internal.unicode_tables; // not lazy, but ok
    import std.internal.unicode_tables : SimpleCaseEntry; // not lazy either :/
}

Technically converting global imports into local or selective ones, is a breaking change due to the broken import system in the compiler see e.g. Use more selective imports at module imports #5584

wilzbach · 2017-12-20T10:43:16Z

continuous-integration/jenkins/pr-merge — This commit cannot be built

Due to a vibe.d test that hangs constantly (not related to this PR) -> dlang/ci#96

jmdavis · 2017-12-20T11:31:53Z

I don't see anything obviously wrong, though it's not surprising if the Windows build broke in the process given that std.datetime.timezone contains Windows-specific code. That's either going to have to be dealt with on a Windows system or risk a fair bit of back and forth with the auto-tester.

I confess thought that this PR helps show why I hate local and selective imports. They help with performance at least some of the time (apparently quite a bit here), so they can be necessary (especially for Phobos, which is imported by everything), and being more explicit about what uses which symbols might help some of the time, but man, the code just gets littered with import statements, and whenever you make changes, you frequently have to muck with the various selective imports instead of just writing code. It would be so much nicer if we could just have the imports at the top without bothering with selective or local imports, but at least right now, the compiler isn't able to do that as efficiently.

andralex · 2017-12-20T21:37:20Z

I confess thought that this PR helps show why I hate local and selective imports. They help with performance at least some of the time (apparently quite a bit here), so they can be necessary (especially for Phobos, which is imported by everything), and being more explicit about what uses which symbols might help some of the time, but man, the code just gets littered with import statements, and whenever you make changes, you frequently have to muck with the various selective imports instead of just writing code. It would be so much nicer if we could just have the imports at the top without bothering with selective or local imports, but at least right now, the compiler isn't able to do that as efficiently.

I'm actually in two minds about this. I am old enough to have seen all of these arguments made earnestly (roughly in chronological order):

"These local variables are so tedious. You mean I need to define i with a type again and again whenever I need to loop over something? I wish I wouldn't need to bother with locals, globals are already there when you need them."
"You mean now some random statement can introduce a new variable? Wish I wouldn't need to bother with that, I prefer it when all variables in a function are nicely declared at the top of the function."
"With virtuals there's no way to tell what code gets executed. Such code is impossible to test!"
"You mean every function call can throw an exception so no more single exit? That's the end of civilization!"

... you get my drift. Local imports are a new technology. I know of no language that does them similarly to the way D does them. It is expected that new technology comes with new advantages, new liabilities, and generally changes in style that are not universally liked.

This PR has been cathartic. Yeah, imports at the top are there for the whole module, but... they're there for the whole module. And I mean all 11,000 lines of it. Initially I thought I'd never figure it out - cutting these mutually recursive imports is surprisingly difficult because compile-time errors don't always give the right information, e.g. when CTFE and constraints are involved. (I will look into filing some bugs.) With this PR it's way more clear what depends on what, where the large dependency knots are, etc.

It may be the case that global imports will become seen as okay for small projects, an antipattern for large programs.

jmdavis · 2017-12-20T22:29:11Z

Yeah, there are pros and cons, and in larger projects, they matter more. I even use them quite a bit even though I hate them. It's just that they get so tedious and repetitive, and when templates aren't involved, there really isn't going to be a performance boost from local imports, since the imports will happen anyway. It's just that it then helps show the dependencies in a more fine-grained manner, which can help avoid importing stuff when you don't need to after refactoring. But if there are a lot of duplicate imports, I'm quickly inclined to put the imports at the top.

In some ways though, the selective imports are far worse than the local imports, because they require more maintenance - and it's particularly bad if you go so far as to use selective imports for the range primitives given how many you end up using for even simple code that uses arrays as ranges. std.range.primitives is one that I pretty much never use selective imports for. In the end, these seem like a prime target for an automated tool, which isn't exactly a good sign. In theory, we're trying to avoid needing extra tools to make code manageable and avoid boilerplate.

If the compiler were smarter and did more imports in a lazy fashion, we could at least mitigate the need to do this for performance, but that still leaves all of the arguments about how much it improves things in terms of how it clarifies dependencies and whether that's worth all of the code duplication.

In any case, as with function attributes, this is one area where good, idiomatic D code seems to be really verbose with the stuff that isn't actual code. And I just can't look at a module with tons of import statements with many of them followed by whole lists of symbols being selectively imported and not get annoyed at how verbose and repetitive it is - especially when the verbosity isn't actually required by the language.

quickfur · 2017-12-20T23:09:53Z

I've been slowly shifting away from global imports in my own code and moving towards local imports. Just like the old C++ innovation of declaring variables close to where you actually need them vs. at the top of the function like older forms of C, I've been finding that local imports are much more well-encapsulated, esp. if put close to or even next to the line where it's actually needed, and blocks of code written this way can be moved around easily without having to constantly fiddle with updating import statements at the top of the scope / module.

Like manually typing long type names in (older forms of?) C++, though, there is definitely a noticeable maintenance cost to it. Which makes me wonder if some import analogue of type inference might be the next step forward. Like some kind of lazy import or auto import. But presently I can't think of any concrete realization of this idea that doesn't either just send us straight back to square one (i.e., declaring dependent modules somehow at the top of the file / start of scope, etc.), or has significant implementational difficulties.

But maybe somebody can take this idea and run with it, and get somewhere new. :-)

andralex · 2017-12-20T23:53:47Z

when templates aren't involved, there really isn't going to be a performance boost from local imports, since the imports will happen anyway.

Nonono. Careful here, I also was confused by that. When you import module xyz, only top-level imports and imports inside aggregates (struct/class/union) are opened. All others are NOT opened. That includes imports inside non-template functions.

MartinNowak · 2017-12-21T13:59:49Z

... you get my drift. Local imports are a new technology. I know of no language that does them similarly to the way D does them. It is expected that new technology comes with new advantages, new liabilities, and generally changes in style that are not universally liked.

Yes, that's how things go. Also new things are often introduced with a more verbose syntax and get simplified later on (e.g delegate (arg) { return expr; } => arg => expr).

In that sentiment, now that things get more adopted, maybe it'd be worth to think about some shorter syntax for local imports. dlang/dmd#6589
I'm personally not a fan of one line per imported module.

Other candidates:
() @trusted { stmt; }(); => @trusted { stmt; }
immutable => imm or let

Not that urgent and a great bike-shedding topic though.

MartinNowak · 2017-12-21T14:03:31Z

std/datetime/systime.d

@@ -8560,6 +8630,7 @@ public:
    // bug# 17801
    @safe unittest
    {
+        import core.time;


How about a top-level version (unittest) import core.time; instead? Maybe even just a selective one?
It's indeed annoying that privately imported modules are resolved eagerly, albeit none of their symbols might ever be needed.

I considered it but am weary of the bugs that could happen. Consider:

version(unittest) import std.stdio; void fun(T)(T value) { writeln(value); } unittest { fun(5); }

This module compiles on its own, passes unittests, is importable when not used, but issues an error whenever the user attempts to use fun. I think we should make it a guideline that version(unittest) top-level imports should be used with caution.

FYI: At least if it's a publicly documented test, this can't happen as we execute them separately exactly for the reason to protect against these kinds of bugs.

I think we should make it a guideline that version(unittest) top-level imports should be used with caution.

It's more or less been a guideline for a while now, precisely because it caused problems in the past. I don't know that we've actually written it down anywhere though, just told people not to do it when it comes up in PRs.

andralex · 2017-12-22T14:20:51Z

@MartinNowak good points. I think we should in the short term move forward with dlang/dmd#6589. No need for a DIP as long as we're removing a limitation.

andralex · 2017-12-23T00:58:42Z

ping?

quickfur

This is a relatively straightforward change with good benefits, and the autotester is green. I say we should just merge.

joakim-noah · 2017-12-23T06:25:13Z

std/datetime/timezone.d

-import std.range.primitives;
+import core.time : abs, convert, dur, Duration, hours, minutes;
+import std.datetime.systime : Clock, stdTimeToUnixTime, SysTime;
+import std.range.primitives : back, front, empty, popFront;


As Seb pointed out, this could break code because of the selective import symbol leak laid out in Issue 17630 and demonstrated in #5584, which is unfixed according to bugzilla. We cannot push all D code to selective imports until all known leaks are fixed, without breaking code.

I've backed off this part of the PR and left a note for the future.

andralex · 2017-12-26T18:19:53Z

squashed, will put up for auto-merge

andralex · 2017-12-26T18:26:13Z

fixed a couple more imports

andralex requested a review from jmdavis as a code owner December 19, 2017 22:20

wilzbach reviewed Dec 20, 2017

View reviewed changes

andralex force-pushed the datetime-imports branch from 57dcc71 to d73e2b2 Compare December 21, 2017 01:12

MartinNowak reviewed Dec 21, 2017

View reviewed changes

wilzbach mentioned this pull request Dec 21, 2017

Remove global debug import in std.typecons #5954

Merged

MartinNowak mentioned this pull request Dec 22, 2017

fix Issue 13855 - multi-module selective import statements dlang/dmd#6589

Merged

quickfur approved these changes Dec 23, 2017

View reviewed changes

joakim-noah reviewed Dec 23, 2017

View reviewed changes

wilzbach mentioned this pull request Dec 25, 2017

Deprecate public aliases for Regex, StaticRegex, etc #5964

Closed

andralex force-pushed the datetime-imports branch 2 times, most recently from b5c7984 to 5bbefaf Compare December 26, 2017 18:19

Improve import speed of std.datetime

cd3152c

andralex force-pushed the datetime-imports branch from 5bbefaf to cd3152c Compare December 26, 2017 18:25

andralex added the auto-merge label Dec 26, 2017

dlang-bot merged commit 279f996 into dlang:master Dec 26, 2017

joakim-noah mentioned this pull request Feb 1, 2018

Use more selective imports at module imports #5584

Closed

d-random-contributor mentioned this pull request Feb 22, 2018

Revert "fix Issue 13855 - multi-module selective import statements" dlang/dmd#7939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dramatically faster to import std.datetime (and anything under it) #5948

Dramatically faster to import std.datetime (and anything under it) #5948

andralex commented Dec 19, 2017

dlang-bot commented Dec 19, 2017

quickfur commented Dec 19, 2017

wilzbach left a comment

wilzbach Dec 20, 2017

wilzbach commented Dec 20, 2017

wilzbach commented Dec 20, 2017

jmdavis commented Dec 20, 2017

andralex commented Dec 20, 2017

jmdavis commented Dec 20, 2017

quickfur commented Dec 20, 2017 •

edited

Loading

andralex commented Dec 20, 2017

MartinNowak commented Dec 21, 2017 •

edited

Loading

MartinNowak Dec 21, 2017

andralex Dec 21, 2017

wilzbach Dec 21, 2017

jmdavis Dec 21, 2017

andralex commented Dec 22, 2017

andralex commented Dec 23, 2017

quickfur left a comment

joakim-noah Dec 23, 2017 •

edited

Loading

andralex Dec 26, 2017

andralex commented Dec 26, 2017

andralex commented Dec 26, 2017

Dramatically faster to import std.datetime (and anything under it) #5948

Dramatically faster to import std.datetime (and anything under it) #5948

Conversation

andralex commented Dec 19, 2017

dlang-bot commented Dec 19, 2017

Bugzilla references

quickfur commented Dec 19, 2017

wilzbach left a comment

Choose a reason for hiding this comment

wilzbach Dec 20, 2017

Choose a reason for hiding this comment

wilzbach commented Dec 20, 2017

wilzbach commented Dec 20, 2017

jmdavis commented Dec 20, 2017

andralex commented Dec 20, 2017

jmdavis commented Dec 20, 2017

quickfur commented Dec 20, 2017 • edited Loading

andralex commented Dec 20, 2017

MartinNowak commented Dec 21, 2017 • edited Loading

MartinNowak Dec 21, 2017

Choose a reason for hiding this comment

andralex Dec 21, 2017

Choose a reason for hiding this comment

wilzbach Dec 21, 2017

Choose a reason for hiding this comment

jmdavis Dec 21, 2017

Choose a reason for hiding this comment

andralex commented Dec 22, 2017

andralex commented Dec 23, 2017

quickfur left a comment

Choose a reason for hiding this comment

joakim-noah Dec 23, 2017 • edited Loading

Choose a reason for hiding this comment

andralex Dec 26, 2017

Choose a reason for hiding this comment

andralex commented Dec 26, 2017

andralex commented Dec 26, 2017

quickfur commented Dec 20, 2017 •

edited

Loading

MartinNowak commented Dec 21, 2017 •

edited

Loading

joakim-noah Dec 23, 2017 •

edited

Loading