Use precompiled headers to shave off ~15% compile time for models. #582
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The pch is too big for CRAN.
…On Nov 2, 2017 6:46 PM, "seantalts" ***@***.***> wrote:
Submisison Checklist
- Run tests: ./runCmdStanTests.py src/test
- Declare copyright holder and open-source license: see below
Summary:
Change CmdStan to use precompiled headers. On my machine this cuts out
about 4-5 seconds of compile time (once everything else is compiled and
we're just compiling and linking the CPP model). I doubt this scales
linearly with more complicated models but who knows! My bernoulli example
takes about 24s without PCH and 19s with. @bgoodri
<https://github.com/bgoodri> might be worth porting to RStan?
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this
will be you or your assignee, such as a university or company): Columbia
University
By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/
licenses/by/4.0/)
------------------------------
You can view, comment on, or merge this pull request online at:
#582
Commit Summary
- Use precompiled headers to shave off ~15% compile time for models.
File Changes
- *M* make/models
<https://github.com/stan-dev/cmdstan/pull/582/files#diff-0> (11)
- *M* makefile
<https://github.com/stan-dev/cmdstan/pull/582/files#diff-1> (1)
Patch Links:
- https://github.com/stan-dev/cmdstan/pull/582.patch
- https://github.com/stan-dev/cmdstan/pull/582.diff
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#582>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADOrqtbqwNTgxbmaCLD5N-4VpinXA8TZks5sykZQgaJpZM4QQcv7>
.
|
|
Can't we build it locally like with this patch? I don't fully understand how RStan works. |
|
CRAN makes a shared object, which last I checked would be about 250MB with
a PCH.
Something like this might work for a package like rstanarm, to create the
PCH first, compile all the models, and then throw the PCH away.
…On Thu, Nov 2, 2017 at 6:53 PM, seantalts ***@***.***> wrote:
Can't we build it locally like with this patch? I don't fully understand
how RStan works.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#582 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADOrqm0ZVp2KIhNflXOmy6do6jmIdKRkks5sykfRgaJpZM4QQcv7>
.
|
|
What does CRAN make a shared object for? I would imagine it would just make one for the stan compiler, but we still need the source code of the stan and math headers on disk on a user's machine in order to compile a Stan model, no? If we have those on disk we can always stop using them as .hpp files and use an amalgamated .hpp.gch precompiled file that we construct as part of the build process instead... I might still be missing something. |
|
@bob-carpenter are you the best person to review this? Or maybe @mitzimorris? |
|
It's probably me. I'll take a look this weekend.
… On Nov 3, 2017, at 11:34 AM, seantalts ***@***.***> wrote:
@bob-carpenter are you the best person to review this? Or maybe @mitzimorris?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Maybe someone else has time to do it today? I have been putting a ton of PRs through to you and I think that's unfair given that this isn't your full time job anymore. |
|
I don't even know what language that's written in!
… On Nov 3, 2017, at 12:00 PM, seantalts ***@***.***> wrote:
Maybe someone else has time to do it today? I have been putting a ton of PRs through to you and I think that's unfair given that this isn't your full time job anymore.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
So an interesting question is - does this even need code review if Daniel was the only one who could read makefiles before? |
|
Strictly speaking I can read that and it looks good. I can't read it well enough that I would trust that it would work as intended without verifying the actual compiler calls that are generated but I assume you did that. |
|
Yep, and all the tests pass, it runs the same number of tests, that kind of thing. Want to approve it for me? We can always back it out later if someone else finds an issue with it. I don't think there's any "CmdStan czar" that needs to see every PR or anything. |
|
Thanks! |
|
awesome! thanks, Krzysztof and Sean!
…On Fri, Nov 3, 2017 at 1:01 PM, seantalts ***@***.***> wrote:
Merged #582 <#582>.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#582 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZ_F9P_JMV0lyfiClwdkrbwSc5eY2dfks5sy0bMgaJpZM4QQcv7>
.
|
|
Oh... I know it's merged now, but could you verify that it works on both Mac and Windows? I thought we had trouble with Windows g++ and precompiled headers. |
|
Funny story - CmdStan does not seem to work on windows before this commit. I checked out the one before:
|
|
You need to add .exe to the end of that command.
Yay, Windows!
…On Fri, Nov 3, 2017 at 2:14 PM, seantalts ***@***.***> wrote:
Funny story - CmdStan does not seem to work on windows before this commit.
I checked out the one before:
[image: image]
<https://user-images.githubusercontent.com/805356/32389512-3a324062-c0a1-11e7-90f1-84824471b5de.png>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#582 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAZ_FywiIV0SqN3eYQUTP8TuuGpP5LNkks5sy1fvgaJpZM4QQcv7>
.
|
|
(╯°□°)╯︵ ┻━┻ <--- Windows |
|
Bahaha. Do executables actually have to end in .exe on Windows??
…On Fri, Nov 3, 2017 at 2:17 PM, Krzysztof Sakrejda ***@***.*** > wrote:
(╯°□°)╯︵ ┻━┻ <--- Windows
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#582 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAxJ7ExVQw4EOVmYRMu1JdyXOsSvXHxoks5sy1jBgaJpZM4QQcv7>
.
|
|
I have to say, that .exe trick works way better. Unfortunately, it still doesn't seem to totally work: What else am I doing wrong? 😅 |
|
Does the libsundials directory exist? Does the library exist? |
|
The `lib` directory under the cvodes directory does not exist. Make does
not appear to have attempted to make them.
…On Fri, Nov 3, 2017 at 4:11 PM, Krzysztof Sakrejda ***@***.*** > wrote:
Does the libsundials directory exist? Does the library exist?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#582 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAxJ7AetsxVlpKWsW4xj-ortyqtpcDWJks5sy3NfgaJpZM4QQcv7>
.
|
|
Should I be using something other than git bash?
…On Fri, Nov 3, 2017 at 4:13 PM, Sean Talts ***@***.***> wrote:
The `lib` directory under the cvodes directory does not exist. Make does
not appear to have attempted to make them.
On Fri, Nov 3, 2017 at 4:11 PM, Krzysztof Sakrejda <
***@***.***> wrote:
> Does the libsundials directory exist? Does the library exist?
>
> —
> You are receiving this because you modified the open/close state.
> Reply to this email directly, view it on GitHub
> <#582 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAxJ7AetsxVlpKWsW4xj-ortyqtpcDWJks5sy3NfgaJpZM4QQcv7>
> .
>
|
|
Did you install g++ via Rtools? |
|
What does git bash have to do with anything? Are you using that instead of command.exe? If so, I don't know how that interacts with make.
Can you try:
make clean-all (or use git to clean)
make build
Then build the executable?
… On Nov 3, 2017, at 4:15 PM, seantalts ***@***.***> wrote:
Should I be using something other than git bash?
On Fri, Nov 3, 2017 at 4:13 PM, Sean Talts ***@***.***> wrote:
> The `lib` directory under the cvodes directory does not exist. Make does
> not appear to have attempted to make them.
>
> On Fri, Nov 3, 2017 at 4:11 PM, Krzysztof Sakrejda <
> ***@***.***> wrote:
>
>> Does the libsundials directory exist? Does the library exist?
>>
>> —
>> You are receiving this because you modified the open/close state.
>> Reply to this email directly, view it on GitHub
>> <#582 (comment)>,
>> or mute the thread
>> <https://github.com/notifications/unsubscribe-auth/AAxJ7AetsxVlpKWsW4xj-ortyqtpcDWJks5sy3NfgaJpZM4QQcv7>
>> .
>>
>
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
I was trying both. `make build` seems to be a necessary precursor step,
even before this PR. Interestingly, the PR does not appear to influence
compilation on Windows at all? I'm not sure how that works, but the PCH
stuff is just totally missing:
[image: Inline image 1]
…On Fri, Nov 3, 2017 at 4:18 PM, Daniel Lee ***@***.***> wrote:
What does git bash have to do with anything? Are you using that instead of
command.exe? If so, I don't know how that interacts with make.
Can you try:
make clean-all (or use git to clean)
make build
Then build the executable?
> On Nov 3, 2017, at 4:15 PM, seantalts ***@***.***> wrote:
>
> Should I be using something other than git bash?
>
> On Fri, Nov 3, 2017 at 4:13 PM, Sean Talts ***@***.***> wrote:
>
> > The `lib` directory under the cvodes directory does not exist. Make
does
> > not appear to have attempted to make them.
> >
> > On Fri, Nov 3, 2017 at 4:11 PM, Krzysztof Sakrejda <
> > ***@***.***> wrote:
> >
> >> Does the libsundials directory exist? Does the library exist?
> >>
> >> —
> >> You are receiving this because you modified the open/close state.
> >> Reply to this email directly, view it on GitHub
> >> <#582 (comment)
>,
> >> or mute the thread
> >> <https://github.com/notifications/unsubscribe-
auth/AAxJ7AetsxVlpKWsW4xj-ortyqtpcDWJks5sy3NfgaJpZM4QQcv7>
> >> .
> >>
> >
> >
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#582 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAxJ7LMrRGoltXeqyLZ6ZBPIVzTYbP5Dks5sy3UYgaJpZM4QQcv7>
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Submisison Checklist
./runCmdStanTests.py src/testSummary:
Change CmdStan to use precompiled headers. On my machine this cuts out about 4-5 seconds of compile time (once everything else is compiled and we're just compiling and linking the CPP model). I doubt this scales linearly with more complicated models but who knows! My bernoulli example takes about 24s without PCH and 19s with. @bgoodri might be worth porting to RStan?
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Columbia University
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: