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

Support skipping non-essential parts of the build #1127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BenWiederhake
Copy link

This significantly speeds up build times for projects which just want to use tdlib as-is.
On a single core this should cut about 5 minutes; on 4 cores it cuts a bit more than a minute.
All this for no measurable cost.

Without skipping:

$ cmake -DCMAKE_BUILD_TYPE=Release -DTD_SKIP_BENCHMARK=OFF -DTD_SKIP_TEST=OFF -DTD_SKIP_TG_CLI=OFF ..
( ... regular output ... )
$ time /usr/bin/time make install -j4 DESTDIR=../../td_destdir
( ... regular output ... )
1414.69user 55.66system 7:01.60elapsed 348%CPU (0avgtext+0avgdata 2752144maxresident)k
0inputs+1210120outputs (0major+21767636minor)pagefaults 0swaps
real	7m1,604s
user	23m34,699s
sys	0m55,665s

With skipping:

$ cmake -DCMAKE_BUILD_TYPE=Release -DTD_SKIP_BENCHMARK=ON -DTD_SKIP_TEST=ON -DTD_SKIP_TG_CLI=ON ..
( ... regular output ... )
$ time /usr/bin/time make install -j4 DESTDIR=../../td_destdir
( ... regular output ... )
1118.16user 42.62system 5:56.22elapsed 325%CPU (0avgtext+0avgdata 2751196maxresident)k
0inputs+891912outputs (0major+17310062minor)pagefaults 0swaps
real	5m56,230s
user	18m38,163s
sys	0m42,629s

This significantly speeds up build times for projects which just want to use tdlib as-is.
On a single core this should cut about 5 minutes; on 4 cores it cuts a bit more than a minute.
All this for no measurable cost.

Without skipping:
$ cmake -DCMAKE_BUILD_TYPE=Release -DTD_SKIP_BENCHMARK=OFF -DTD_SKIP_TEST=OFF -DTD_SKIP_TG_CLI=OFF ..
( ... regular output ... )
$ time /usr/bin/time make install -j4 DESTDIR=../../td_destdir
( ... regular output ... )
1414.69user 55.66system 7:01.60elapsed 348%CPU (0avgtext+0avgdata 2752144maxresident)k
0inputs+1210120outputs (0major+21767636minor)pagefaults 0swaps
real	7m1,604s
user	23m34,699s
sys	0m55,665s

With skipping:
$ cmake -DCMAKE_BUILD_TYPE=Release -DTD_SKIP_BENCHMARK=ON -DTD_SKIP_TEST=ON -DTD_SKIP_TG_CLI=ON ..
( ... regular output ... )
$ time /usr/bin/time make install -j4 DESTDIR=../../td_destdir
( ... regular output ... )
1118.16user 42.62system 5:56.22elapsed 325%CPU (0avgtext+0avgdata 2751196maxresident)k
0inputs+891912outputs (0major+17310062minor)pagefaults 0swaps
real	5m56,230s
user	18m38,163s
sys	0m42,629s
@levlam
Copy link
Contributor

levlam commented Jul 6, 2020

This reduces build time only by 15% in your example. I'm not sure that such small speed up worth build setup complication, especially given build time can be improved much more without any change. You need to build TDLib only once, so build time should be irrelevant for ordinary usages, and for development purposes it is important to check that everything is built correctly. I also wouldn't call tests a non-essential part.

To build a specific target it is better to explicitly specify it, for example, cmake --build . --target tdjson -- -j4 or make -j4 tdjson. Install target wouldn't work this way, but it is easy to manually copy libtdjson.so or any other needed file.

@BenWiederhake
Copy link
Author

build setup complication

I don't really see how this is a complication.

should be irrelevant for ordinary usages

It saves 5 minutes on Travis every time we need to rebuild. (Also because Travis only goes to -j 2.)
For the cost of 5 additional lines, and 3 self-explaining options.

I also wouldn't call tests a non-essential part.

You're right, I shouldn't have called it non-essential. However, I hope you can see that someone who just wants to install it (and not run tests or benchmarks) may want to cut down those 5 minutes.

it is easy to manually copy libtdjson.so

It's also very brittle: This would mean that we ignore td's build system, and "guess" what the install target needs, and do all that manually, even though there is already an obvious place where definitions like this should go: td's CMakeLists.txt.

Effectively I would need to check every now and then whether our "guess" is still "correct enough", which would be a non-automatable process.

@isopen
Copy link
Contributor

isopen commented Jul 7, 2020

@BenWiederhake
Copy link
Author

You can try ninja

What? Is that a serious comment? How does this solve anything?

@levlam
Copy link
Contributor

levlam commented Jul 7, 2020

@BenWiederhake

build setup complication

I don't really see how this is a complication.

If it is not properly documented, then almost noone will use it, so it is useless. If it is properly documented, then it is a complication, because everyone will need to read about it.

should be irrelevant for ordinary usages

It saves 5 minutes on Travis every time we need to rebuild. (Also because Travis only goes to -j 2.)
For the cost of 5 additional lines, and 3 self-explaining options.

Unless you are going to test TDLib build or run TDLib tests, there is no reason to rebuild TDLib every time. It is much simpler and efficient to create a Docker container with prebuilt TDLib or use https://docs.travis-ci.com/user/caching/#caching-directories-bundler-dependencies.

I thought a few times about conditional building of some TDLib parts, but all the time when this was an acceptable solution, there was a simple way to achieve better result.

@camapantap
Copy link

Code

@r3z4r3557
Copy link

Hii

@mbasaglia
Copy link
Contributor

If it is not properly documented, then almost noone will use it, so it is useless. If it is properly documented, then it is a complication, because everyone will need to read about it.

CMake options have built-in documentation, having like 8 extra lines in the CMake file to reduce every build by 15% seems a pretty good deal to me...

Also, not everyone needs to know about them, just those who look through the list of options if they want to customize the build.

The CMake file already has a bunch of options so I'm not sure why adding 3 more to speed up compilation adds complexity if I must be honest

@levlam
Copy link
Contributor

levlam commented Aug 2, 2021

@mbasaglia If you know that you can see CMake options, you should also know that you can build a single target with CMake.

Also, not everyone needs to know about them, just those who look through the list of options if they want to customize the build.

That's exactly the reason why they will be useless. 1% of developers will do that, but even these developers should be able to customize CMakeLists.txt themselves in no time.

@mbasaglia
Copy link
Contributor

Customizing CMakeLists.txt in CI is not viable, also I would need to know which targets are needed...

@tdlib tdlib deleted a comment Dec 24, 2021
@0asi0
Copy link

0asi0 commented Jun 1, 2022

This significantly speeds up build times for projects which just want to use tdlib as-is.

On a single core this should cut about 5 minutes; on 4 cores it cuts a bit more than a minute.

All this for no measurable cost.

Without skipping:


$ cmake -DCMAKE_BUILD_TYPE=Release -DTD_SKIP_BENCHMARK=OFF -DTD_SKIP_TEST=OFF -DTD_SKIP_TG_CLI=OFF ..

( ... regular output ... )

$ time /usr/bin/time make install -j4 DESTDIR=../../td_destdir

( ... regular output ... )

1414.69user 55.66system 7:01.60elapsed 348%CPU (0avgtext+0avgdata 2752144maxresident)k

0inputs+1210120outputs (0major+21767636minor)pagefaults 0swaps

real	7m1,604s

user	23m34,699s

sys	0m55,665s

With skipping:


$ cmake -DCMAKE_BUILD_TYPE=Release -DTD_SKIP_BENCHMARK=ON -DTD_SKIP_TEST=ON -DTD_SKIP_TG_CLI=ON ..

( ... regular output ... )

$ time /usr/bin/time make install -j4 DESTDIR=../../td_destdir

( ... regular output ... )

1118.16user 42.62system 5:56.22elapsed 325%CPU (0avgtext+0avgdata 2751196maxresident)k

0inputs+891912outputs (0major+17310062minor)pagefaults 0swaps

real	5m56,230s

user	18m38,163s

sys	0m42,629s

https://t.me/sumisaliii

@eugenebas
Copy link

I need that options, please merge them.

@levlam
Copy link
Contributor

levlam commented Aug 4, 2022

@eugenebas As explained in #1127 (comment) and #1127 (comment), if you need these options, you are doing something wrong. What is you use case?

@andrew-phi
Copy link

It would be nice to have benchmark and test projects DISABLED by default and parallel building ENABLED! It's 2022 and people use multicore systems mostly.

@levlam
Copy link
Contributor

levlam commented Aug 14, 2022

@andrew-phi Enabled multicore build by default would lead only to failed build by default. If you are sure that you have enough RAM for multicore build, the you are free to pass corresponding argument to build system.

@andrew-phi
Copy link

@levlam if it would fail, it would fail faster than waiting many hours just to get really, REALLY frustrated (I waited ~3h for 70%, 4cores/6GB). I'm not familiar with cmake, but cmake --build . --target install -- -j 4 seems like a hack to me. Or at least a proper way of doing so should be mentioned / added to build.html as an option.

@levlam
Copy link
Contributor

levlam commented Aug 15, 2022

4cores/6GB). I'm not familiar with cmake, but cmake --build . --target install -- -j 4

This is exactly the command, which must not be advertised. GCC needs about 4GB per core, so the mentioned command will trigger OOM, killing random processes on your server. It have no chance to succeed with 6GB RAM.

@tdlib tdlib deleted a comment from amir232312 Oct 1, 2022
Copy link

@Nuttapon-Makchoos Nuttapon-Makchoos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

@Nuttapon-Makchoos Nuttapon-Makchoos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tdlib tdlib deleted a comment from Nuttapon-Makchoos Nov 5, 2022
@tdlib tdlib deleted a comment from Vladimirovdent Jan 21, 2023
@tdlib tdlib deleted a comment from Vladimirovdent Jan 21, 2023
@tdlib tdlib deleted a comment from Vladimirovdent Jan 21, 2023
@tdlib tdlib deleted a comment from Ataif Feb 27, 2023
Copy link

@ivanstepanovftw ivanstepanovftw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move options to one place

@aaalzya aaalzya mentioned this pull request Jun 13, 2023
aaalzya

This comment was marked as off-topic.

aaalzya

This comment was marked as off-topic.

@tdlib tdlib deleted a comment Jun 14, 2023
@asarubbo
Copy link

See: #2504 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet