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

Build phobos package at a time #3379

Merged
merged 2 commits into from Jun 6, 2015
Merged

Conversation

andralex
Copy link
Member

@andralex andralex commented Jun 6, 2015

On my Powerbook, ran the command:

make clean && /usr/bin/time -lp make -j6 BUILD=debug MODEL=64

Took the best of three runs for baseline and proposed.

Before: 4.93s, 960270336 maximum resident set size
After: 3.85s, 618004480 maximum resident set size

On a big iron at work running Centos 6, ran the command:

make clean && /usr/bin/time -f "%P %M" make -j64 BUILD=debug MODEL=64 lib

Took the best of three.

Before: 5.12s, 1191664
After: 3.86s, 767252

@andralex
Copy link
Member Author

andralex commented Jun 6, 2015

Forgot to build and include the extra files. This has low impact of performance as follows:

OSX: 4.01s, 617668608
Centos: 3.85s, 767256

CyberShadow added a commit that referenced this pull request Jun 6, 2015
@CyberShadow CyberShadow merged commit a928a21 into dlang:master Jun 6, 2015
@MartinNowak
Copy link
Member

Sure this was worth the trouble? Combining all the static libs already takes 300ms on my machine.

@HK47196
Copy link
Contributor

HK47196 commented Jun 30, 2015

This commit increased the size of libphobos2.a by 16MiB(~20%) according to http://digger.k3.1azy.net/trend/

@CyberShadow
Copy link
Member

Well spotted, @rsw0x.

This is bad. I think we should revert this. A 300ms build time improvement does not justify increasing the download size.

@quickfur
Copy link
Member

Yeah, this is very bad. Please revert this.

@CyberShadow
Copy link
Member

Well, this can't be reverted automatically now.

Anyone up for Makefile surgery?

9rnsr added a commit to 9rnsr/phobos that referenced this pull request Jul 1, 2015
@9rnsr 9rnsr mentioned this pull request Jul 1, 2015
@9rnsr
Copy link
Contributor

9rnsr commented Jul 1, 2015

@MartinNowak @rsw0x @quickfur @CyberShadow I opened #3461 to try revert this PR. Could you test that will correctly fix the problem?

@9rnsr
Copy link
Contributor

9rnsr commented Jul 1, 2015

And to @andralex , please review #3461.

9rnsr added a commit to 9rnsr/phobos that referenced this pull request Jul 7, 2015
9rnsr added a commit to 9rnsr/phobos that referenced this pull request Jul 9, 2015
MartinNowak added a commit to MartinNowak/phobos that referenced this pull request Jul 23, 2015
9rnsr added a commit that referenced this pull request Jul 23, 2015
Revert "Merge pull request #3379 from andralex/build-per-package"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants