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

Modularize the CMake build targets of each game #1124

Closed

Conversation

jthemphill
Copy link

This PR modularizes the dependencies for amazons. If you approve of my general direction, I'll extend this to all the games, which should cut the test folder's size from 600MB to 100MB and reduce test linkage time by 33%.

While trying to build the test for one of the games, I noticed the Makefile had me building every game:

jhemphill@CeilingComp:~/oss/open_spiel/build$ make -j$(nproc) amazons_test
[  0%] Built target absl_spinlock_wait
...
[ 52%] Built target absl_flags_parse
Consolidate compiler generated dependencies of target bots
Consolidate compiler generated dependencies of target utils
Consolidate compiler generated dependencies of target game_transforms
Consolidate compiler generated dependencies of target bridge_double_dummy_solver
[ 52%] Built target tests
Consolidate compiler generated dependencies of target algorithms
[ 52%] Built target bots
[ 54%] Built target utils
[ 54%] Building CXX object game_transforms/CMakeFiles/game_transforms.dir/start_at.cc.o
...
[ 58%] Building CXX object games/CMakeFiles/games.dir/battleship/battleship.cc.o
[ 62%] Building CXX object games/CMakeFiles/bridge_double_dummy_solver.dir/bridge/double_dummy_solver/src/TimerList.cpp.o
[ 62%] Building CXX object algorithms/CMakeFiles/algorithms.dir/evaluate_bots.cc.o
[ 62%] Building CXX object games/CMakeFiles/games.dir/blackjack/blackjack.cc.o
...

In addition to building everything, it seems to have linked everything in as well, resulting in a 7.4MB test:

jhemphill@CeilingComp:~/oss/open_spiel/build$ ls -lh games/amazons_test 
-rwxr-xr-x 1 jhemphill jhemphill 7.4M Oct 14 17:23 games/amazons_test

Since every test links every game in, every test is at least 7.4MB:

jhemphill@CeilingComp:~/oss/open_spiel/build$ ls -lh games/
total 608M
-rwxr-xr-x  1 jhemphill jhemphill 7.4M Oct 14 12:36 2048_test
drwxr-xr-x 86 jhemphill jhemphill 4.0K Oct 14 17:15 CMakeFiles
-rw-r--r--  1 jhemphill jhemphill  21K Oct 14 17:15 CTestTestfile.cmake
-rw-r--r--  1 jhemphill jhemphill 312K Oct 14 17:15 Makefile
-rwxr-xr-x  1 jhemphill jhemphill 7.4M Oct 14 17:23 amazons_test
-rwxr-xr-x  1 jhemphill jhemphill 7.5M Oct 14 12:36 backgammon_test
...
-rwxr-xr-x  1 jhemphill jhemphill 7.7M Oct 14 17:27 tarok_test
-rwxr-xr-x  1 jhemphill jhemphill 7.4M Oct 14 12:37 tic_tac_toe_test
-rwxr-xr-x  1 jhemphill jhemphill 7.4M Oct 14 12:37 tiny_bridge_test
-rwxr-xr-x  1 jhemphill jhemphill 7.4M Oct 14 12:37 tiny_hanabi_test
-rwxr-xr-x  1 jhemphill jhemphill 7.4M Oct 14 12:37 trade_comm_test
-rwxr-xr-x  1 jhemphill jhemphill 7.4M Oct 14 12:37 ultimate_tic_tac_toe_test
-rwxr-xr-x  1 jhemphill jhemphill 7.4M Oct 14 12:37 y_test

The directory is 608MB, which is what we expect from 80 tests, each 7.5MB:

jhemphill@CeilingComp:~/oss/open_spiel/build$ ls games/*_test | wc -l
80
jhemphill@CeilingComp:~/oss/open_spiel/build$ python3 -c 'print(80 * 7.5)'
600.0

Removing redundant linkage reduces the size of amazons_test by a factor of 5:

jhemphill@CeilingComp:~/oss/open_spiel/build$ ls -lh games/amazons_test 
-rwxr-xr-x 1 jhemphill jhemphill 1.3M Oct 14 17:32 games/amazons_test

And cuts a third off of the link time of amazons_test:

# Before (with all object files prebuilt)
jhemphill@CeilingComp:~/oss/open_spiel/build$ rm games/amazons_test && time make -j$(nproc) amazons_test
real    0m1.487s
user    0m1.961s
sys     0m0.688s
# After (with all object files prebuilt)
jhemphill@CeilingComp:~/oss/open_spiel/build$ rm games/amazons_test && time make -j$(nproc) amazons_test
real    0m0.863s
user    0m1.411s
sys     0m0.449s

I expect this change will do the same for each test.

@google-cla
Copy link

google-cla bot commented Oct 15, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@lanctot
Copy link
Collaborator

lanctot commented Oct 15, 2023

Wow, this is great, thanks!

Yes, it would be a great thing to do for all games (some of those tests will depend on multiple games, and if there's no easy way to do it we can keep it linking to everything for those instances)

@jthemphill
Copy link
Author

Well, this version of the PR creates a .a library file for each game, which regresses build times and creates a bunch of duplicated symbols.

The old version of the code, which links everything in:

$ make clean ; time make -j8
real    5m25.136s
user    34m56.492s
sys     2m8.369s

$ du -sh games
629M    games

The version I just put up, with modularized dependencies:

$ make clean ; time make -j8
real    6m6.244s
user    41m23.430s
sys     2m17.900s
(venv) jhemphill@CeilingComp:~/oss/open_spiel/build$ du -sh games
355M    games

But there's a third way, which leads to faster build times and less maintenance to add new games. If we just link all the games, and all the testing dependencies, into a libgames.a library file:

$ make clean ; time make -j8
real    5m3.767s
user    32m50.052s
sys     1m53.803s

$ du -sh games
208M    games

@jthemphill
Copy link
Author

Check out #1126, which is substantially simpler and actually does improve the build time this time😅

@jthemphill
Copy link
Author

Closing in favor of #1126.

@jthemphill jthemphill closed this Oct 17, 2023
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.

2 participants