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

std.range.constraints & std.array code/import swap #2701

Merged
merged 1 commit into from Nov 14, 2014
Merged

std.range.constraints & std.array code/import swap #2701

merged 1 commit into from Nov 14, 2014

Conversation

9il
Copy link
Member

@9il 9il commented Nov 13, 2014

The goal of this PR is to prevent import std.array, std.functional and std.algorithm through std.range.constraints.

  1. clean imports in std.array
  2. move some common code into std.range.constraints. The reason is optimisation of compilation time.

Note:

  1. std.array imports std.functional and std.algorithm in global scope. See std.array.splitter.
  2. std.range.constraints is going to be common global import for half of phobos instead of std.range.

See also: #2700 #2661 Issue 13253 - use more scoped imports in phobos

@9il 9il mentioned this pull request Nov 13, 2014
@quickfur
Copy link
Member

I'm unsure about putting array range primitives in std.range.constraints. Array range primitives are not constraints, and std.range.constraints is growing to include lots of other non-constraint stuff. Maybe we should rename it to std.range.primitives instead, or maybe make that a separate submodule of std.range?

@9il
Copy link
Member Author

9il commented Nov 13, 2014

Both range and array primitives should be imported into std.range.constraints because is* and has* use primitives.
If you use std.range.constraints you would use primitives (98%).
std.array globally include std.range.constraints anyway.

So I think this PR is good. It also do not break any code.

@DmitryOlshansky
Copy link
Member

@quickfur The name is unfortunate but let's not dwell on that. Think of it as core or minimal.

@DmitryOlshansky
Copy link
Member

LGTM

@DmitryOlshansky
Copy link
Member

@9il Would be cool to see some stats on build times. I'll collect some on my own when time permits but if you did such measures would be nice to share them.

@9il
Copy link
Member Author

9il commented Nov 13, 2014

@DmitryOlshansky I know how to build dmd with Digger manager. How can do this with my local repo?

@DmitryOlshansky
Copy link
Member

@9il technically Digger should accept old pull numbers and hashes of commits as well?

But anyway:

git checkout <hash of some commit before your pulls>
make -f posix.mak
time make -f posix.mak unittest

#then back to master
git checkout master
make -f posix.mak
time make -f posix.mak unittest

@9il
Copy link
Member Author

9il commented Nov 13, 2014

I think test with unittest will have the same time because:

  1. Files in posix.mak are grouped into few parts.
  2. unittest version of code often imports half of phobos.

@9il
Copy link
Member Author

9il commented Nov 13, 2014

This stuff should be compiled faster with PR.
time dmd -main test.d

import std.range.constraints;

auto foo(Range, E)(Range r, E e) if(isInputRange!Range)
{
    while(!r.empty && r.front != e)
        r.popFront;
    return r;
}

alias bar = foo!(uint[], uint);

@DmitryOlshansky
Copy link
Member

All in all things seem to improve, or at least not regress.

a728319f8bbae9a2b57d0d4fd6133bc3d66d4d75:
time make -f posix.mak
real    0m11.299s
user    0m9.077s
sys 0m2.109s
real    0m11.376s
user    0m9.137s
sys 0m2.108s

unittest
real    2m33.675s
user    2m6.192s
sys 0m26.322s

master:

time make -f posix.mak
real    0m11.171s
user    0m8.994s
sys 0m2.066s

real    0m11.376s
user    0m9.119s
sys 0m2.116s

unittest
real    2m29.171s
user    2m3.104s
sys 0m25.652s

@DmitryOlshansky
Copy link
Member

Aaand your test case, before this pull:

dmitry@Ubu64 ~/dmd2/src/phobos $ time dmd -main testing.d 

real    0m0.234s
user    0m0.132s
sys 0m0.065s
dmitry@Ubu64 ~/dmd2/src/phobos $ time dmd -main testing.d 

real    0m0.211s
user    0m0.132s
sys 0m0.065s

After:

dmitry@Ubu64 ~/dmd2/src/phobos $ time dmd -main testing.d 

real    0m0.187s
user    0m0.108s
sys 0m0.054s
dmitry@Ubu64 ~/dmd2/src/phobos $ time dmd -main testing.d 

real    0m0.165s
user    0m0.111s
sys 0m0.054s
dmitry@Ubu64 ~/dmd2/src/phobos $ time dmd -main testing.d 

real    0m0.167s
user    0m0.112s
sys 0m0.052s

@9il
Copy link
Member Author

9il commented Nov 13, 2014

@DmitryOlshansky Thanks!

use static import

rm static
@quickfur
Copy link
Member

Anything else to address? Are we ready to merge?

@quickfur
Copy link
Member

P.S. Regarding the naming of std.range.constraints, it doesn't directly concern this PR, so it doesn't need to block this PR, but we do need to seriously consider a proper name for it before the next release. We can still rename it now without major problems, but once it makes it into an official release, it's going to be much harder to change things. So we better think very carefully now what name we want to use, while it's still relatively easy to change.

@9il
Copy link
Member Author

9il commented Nov 14, 2014

Yes, I ready)

@9il
Copy link
Member Author

9il commented Nov 14, 2014

@quickfur Please take under control the question about module name and discus it with other collaborators.

@9il 9il mentioned this pull request Nov 14, 2014
@quickfur
Copy link
Member

Maybe we should bring it up in the forum?

@quickfur
Copy link
Member

Auto-merge toggled on

quickfur pushed a commit that referenced this pull request Nov 14, 2014
std.range.constraints & std.array code/import swap
@quickfur quickfur merged commit abc3fb1 into dlang:master Nov 14, 2014
@9il 9il deleted the ar branch November 14, 2014 05:22
@9il
Copy link
Member Author

9il commented Nov 14, 2014

@quickfur At your convenience (about forum).

@9il 9il mentioned this pull request Nov 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants