Skip to content

build: add LTO option to depends #23611

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 14, 2022
Merged

build: add LTO option to depends #23611

merged 1 commit into from
Jun 14, 2022

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Nov 27, 2021 •
edited
Loading

This adds an LTO option to depends, i.e make -C depends LTO=1, which passes -flto when building packages (not currently qt), and automatically configures with --enable-lto when doing a build using a CONFIG_SITE.

The following tables comapres the size (in bytes) of the stripped x86_64 Linux binaries produced with master and this PR (full depends build):

Binary stripped master stripped LTO=1 saving
bitcoin-cli 1178632 469872 60%
bitcoin-tx 2710584 1866504 31%
bitcoin-util 952880 240104 74%
bitcoin-wallet 7992888 5365984 32%
bitcoind 13421336 11868592 12%
bitcoin-qt 37680496 31640976 16%

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 27, 2021 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23609 (build: Add -fvisibility=hidden flag for macOS host by hebasto)
  • #22555 (build: Fix make apk for Android w/ non-default SOURCES_PATH in depends by hebasto)
  • #22380 (build: add and use C_STANDARD and CXX_STANDARD in depends by fanquake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@jarolrod
Copy link
Member

I haven't investigated yet, and it's possible either may be fixed after #23489

Cherry picked this on top of #23489, the errors with qt cross-compile to windows and macOS are still there. Windows error should be an easy fix

This was referenced Nov 28, 2021
Merged
Closed
Closed
@DrahtBot DrahtBot mentioned this pull request Dec 13, 2021
Merged
This was referenced Dec 20, 2021
Merged
Closed
@DrahtBot DrahtBot mentioned this pull request Jan 4, 2022
Merged
@fanquake fanquake changed the title build: add LTO option to depends build: add LTO option to depends (no Qt) Feb 3, 2022
@fanquake
Copy link
Member Author

fanquake commented Feb 3, 2022

Rebased, updated for FreeBSD, and dropped the use of Qts -ltcg option for now.

@fanquake fanquake added this to the 24.0 milestone Feb 23, 2022
@fanquake
Copy link
Member Author

Rebased, and put on top of #24604.

@hebasto
Copy link
Member

hebasto commented Mar 25, 2022

Concept ACK.

@fanquake
Copy link
Member Author

Rebased now that #24604 is merged. Some outstanding issues:

on macOS, our Qt configure checks fail when building with -flto:

configure:31764: checking for QMinimalIntegrationPlugin (-lqminimal)
...
ld: warning: duplicate symbol 'lcQpaFonts()' in:
    bitcoin/depends/x86_64-apple-darwin21.4.0/lib/libQt5ThemeSupport.a(qgenericunixthemes.o)
    bitcoin/depends/x86_64-apple-darwin21.4.0/lib/libQt5FontDatabaseSupport.a(qfontengine_coretext.o)
ld: Linking globals named '_Z10lcQpaFontsv': symbol multiply defined! for architecture x86_64

On Ubuntu, running test/util/test_runner.py fails (unit and functional tests pass):

Running tests: db_tests from wallet/test/db_tests.cpp
Running test/util/test_runner.py...
/usr/bin/python3.8 ../test/util/test_runner.py
2022-03-25 08:58:33,775 - ERROR - Error mismatch:
Expected: error: TX output missing or too many separators
Received: error: Unknown error -1
2022-03-25 08:58:33,777 - ERROR - Error mismatch:
Expected: error: TX output missing or too many separators
Received: error: Unknown error -1
2022-03-25 08:58:33,940 - ERROR - Output data mismatch for txcreatesignv1.hex (format hex)
2022-03-25 08:58:33,940 - ERROR - Output formatting mismatch for txcreatesignv1.hex:
*** txcreatesignv1.hex
--- returned
***************
*** 1 ****
- 01000000018594c5bdcaec8f06b78b596f31cd292a294fd031e24eec716f43dac91ea7494d000000008a4730440220131432090a6af42da3e8335ff110831b41a44f4e9d18d88f5d50278380696c7202200fc2e48938f323ad13625890c0ea926c8a189c08b8efc38376b20c8a2188e96e01410479be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8ffffffff01a0860100000000001976a9145834479edbbe0539b31ffd3a8f8ebadc2165ed0188ac00000000
--- 0 ----

2022-03-25 08:58:33,942 - ERROR - Error parsing command output as json: Expecting value: line 1 column 1 (char 0)
2022-03-25 08:58:33,945 - ERROR - Output data mismatch for txcreatesignv2.hex (format hex)
2022-03-25 08:58:33,945 - ERROR - Output formatting mismatch for txcreatesignv2.hex:
*** txcreatesignv2.hex
--- returned
***************
*** 1 ****
- 02000000018594c5bdcaec8f06b78b596f31cd292a294fd031e24eec716f43dac91ea7494d000000008a473044022079c7aa014177a2e973caf6df7c7b8f15399083b91eba370ea1e19c4caed9181e02205f8f8763505ce8e6cbdd2cd28fab3fd407a75003e7d0dc04e6bebb0a3c89e7cb01410479be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8ffffffff01a0860100000000001976a9145834479edbbe0539b31ffd3a8f8ebadc2165ed0188ac00000000
--- 0 ----

2022-03-25 08:58:33,947 - ERROR - Error mismatch:
Expected: error: prevtxs internal object typecheck fail
Received: error: Unknown error -1
2022-03-25 08:58:33,949 - ERROR - Error mismatch:
Expected: error: txid must be hexadecimal string (not 'Zd49a71ec9da436f71ec4ee231d04f292a29cd316f598bb7068feccabdc59412')
Received: error: Unknown error -1
2022-03-25 08:58:33,951 - ERROR - Error mismatch:
Expected: error: txid must be hexadecimal string (not '4d49a71ec9da436f71ec4ee231d04f292a29cd316f598bb7068feccabdc594')
Received: error: Unknown error -1
2022-03-25 08:58:33,953 - ERROR - Error mismatch:
Expected: error: txid must be hexadecimal string (not '4d49a71ec9da436f71ec4ee231d04f292a29cd316f598bb7068feccabdc5948512')
Received: error: Unknown error -1
2022-03-25 08:58:33,956 - ERROR - Output data mismatch for txcreatesignsegwit1.hex (format hex)
2022-03-25 08:58:33,956 - ERROR - Output formatting mismatch for txcreatesignsegwit1.hex:
*** txcreatesignsegwit1.hex
--- returned
***************
*** 1 ****
- 02000000000101ffeeddccbbaa99887766554433221100ffeeddccbbaa998877665544332211000000000000ffffffff01a0860100000000001976a9145834479edbbe0539b31ffd3a8f8ebadc2165ed0188ac0247304402202e8d8677912f73909ffbdb3ee87d10cce41d398ee206e534fa18330b566ece34022004f944f018a03c9f5b4cf0e9b0ae4f14049b55e7b6810a6ac26cd67cb4dcb31f01210279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f8179800000000
--- 0 ----

2022-03-25 08:58:33,958 - ERROR - Error mismatch:
Expected: Missing amount for CTxOut with scriptPubKey=0014751e76e8199196d454941c45d1b3a323f1433bd6
Received: error: Unknown error -1
2022-03-25 08:58:33,961 - ERROR - Output data mismatch for txcreateoutpubkey1.hex (format hex)
2022-03-25 08:58:33,961 - ERROR - Output formatting mismatch for txcreateoutpubkey1.hex:
*** txcreateoutpubkey1.hex
--- returned
***************
*** 1 ****
- 0100000000010000000000000000232102a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff397ac00000000
--- 0 ----

2022-03-25 08:58:33,964 - ERROR - Error parsing command output as json: Expecting value: line 1 column 1 (char 0)
2022-03-25 08:58:33,966 - ERROR - Output data mismatch for txcreateoutpubkey2.hex (format hex)
2022-03-25 08:58:33,966 - ERROR - Output formatting mismatch for txcreateoutpubkey2.hex:
*** txcreateoutpubkey2.hex
--- returned
***************
*** 1 ****
- 0100000000010000000000000000160014a2516e770582864a6a56ed21a102044e388c62e300000000
--- 0 ----

2022-03-25 08:58:33,969 - ERROR - Error parsing command output as json: Expecting value: line 1 column 1 (char 0)
2022-03-25 08:58:33,972 - ERROR - Output data mismatch for txcreateoutpubkey3.hex (format hex)
2022-03-25 08:58:33,972 - ERROR - Output formatting mismatch for txcreateoutpubkey3.hex:
*** txcreateoutpubkey3.hex
--- returned
***************
*** 1 ****
- 010000000001000000000000000017a914a5ab14c9804d0d8bf02f1aea4e82780733ad0a838700000000
--- 0 ----

2022-03-25 08:58:33,975 - ERROR - Error parsing command output as json: Expecting value: line 1 column 1 (char 0)
2022-03-25 08:58:33,977 - ERROR - Error mismatch:
Expected: error: Uncompressed pubkeys are not useable for SegWit outputs
Received: error: Unknown error -1
2022-03-25 08:58:34,013 - ERROR - Error mismatch:
Expected: error: invalid multisig required number '-2'
Received: error: Unknown error -1
2022-03-25 08:58:34,015 - ERROR - Error mismatch:
Expected: error: invalid multisig total number '3a'
Received: error: Unknown error -1
2022-03-25 08:58:34,017 - ERROR - Output data mismatch for txcreatemultisig1.hex (format hex)
2022-03-25 08:58:34,018 - ERROR - Output formatting mismatch for txcreatemultisig1.hex:
*** txcreatemultisig1.hex
--- returned
***************
*** 1 ****
- 01000000000100e1f5050000000069522102a5613bd857b7048924264d1e70e08fb2a7e6527d32b7ab1bb993ac59964ff39721021ac43c7ff740014c3b33737ede99c967e4764553d1b2b83db77c83b8715fa72d2102df2089105c77f266fa11a9d33f05c735234075f2e8780824c6b709415f9fb48553ae00000000
--- 0 ----

@fanquake
Copy link
Member Author

Rebased past #19952. The issue with Qt on macOS should now also be fixed.

This was referenced Apr 16, 2022
Merged
Merged
@fanquake
Copy link
Member Author

fanquake commented May 5, 2022

Rebased past #24866.

@fanquake fanquake marked this pull request as ready for review May 5, 2022 12:25
prusnak
prusnak approved these changes May 5, 2022
View reviewed changes
Copy link
Contributor

@prusnak prusnak left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approach ACK 8af8cf8

@fanquake
Copy link
Member Author

fanquake commented Jun 8, 2022

Fixed depends when using LTO.

@laanwj
Copy link
Member

laanwj commented Jun 14, 2022

Concept ACK. Going to test on RISC-V 64-bit.

@fanquake fanquake changed the title build: add LTO option to depends (no Qt) build: add LTO option to depends (no depends Qt) Jun 14, 2022
@fanquake fanquake changed the title build: add LTO option to depends (no depends Qt) build: add LTO option to depends Jun 14, 2022
@fanquake
Copy link
Member Author

I've rebase this branch on master, and updated and simplified the savings table in the PR description. I now just compare the stripped master vs stripped LTO sizes. I've also re-added bitcoin-qt. As of the current branch, when compiled using GCC 11.2.0, the flto savings for bitcoind and bitcoin-qt are currently 12% and 16%

@laanwj
Copy link
Member

laanwj commented Jun 14, 2022

I've checked that the generated .a files contain LTO objects after building depends with LTO=1, and that the build still works. I did not yet get around to benchmark anything, or compare file sizes, but that's not a blocker for adding this option.

Tested ACK 0947726

@laanwj laanwj merged commit df93359 into bitcoin:master Jun 14, 2022
@fanquake fanquake deleted the lto_in_depends branch June 14, 2022 19:48
@laanwj
Copy link
Member

laanwj commented Jun 15, 2022

(all results for RISC-V 64-bit, SiFive U740)
i did three builds; one with full LTO (both depends and bitcoin build), "half LTO" (only for bitcoin build, normal depends), and non LTO
These are the resulting binary sizes:

  7151688 bin.lto.stripped/bitcoind (-35%)
  9517544 bin.halflto.stripped/bitcoind (-14%)
 11035608 bin.nonlto.stripped/bitcoind

Clearly, it makes a lot of difference! Resulting binaries all work.

I also compared two benchmark runs (full LTO versus no LTO). There's not much difference for most of them, though these stand out:

ns/op op/s err% ins/op cyc/op bra/op miss% total benchmark
2,464,870,970.00 0.41 0.3% 0.00 0.00 0.00 0.0% 27.13 ComplexMemPool (lto)
2,794,290,011.00 0.36 0.2% 0.00 0.00 0.00 0.0% 30.71 ComplexMemPool (no lto)

At least nothing is significantly slower either.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 15, 2022
@laanwj
Merge bitcoin#23611: build: add LTO option to depends
739357e 
0947726 build: support LTO in depends (fanquake)

Pull request description:

  This adds an `LTO` option to depends, i.e `make -C depends LTO=1`, which passes `-flto` when building packages (not currently qt), and automatically configures with `--enable-lto` when doing a build using a `CONFIG_SITE`.

  The following tables comapres the size (in bytes) of the stripped `x86_64` Linux binaries produced with master and this PR (full depends build):

  | Binary | stripped master | stripped LTO=1 | saving |
  | -------- | ----------------: | -------------: | --------: |
  | bitcoin-cli | 1178632 | 469872 | 60% |
  | bitcoin-tx  | 2710584 | 1866504 | 31% |
  | bitcoin-util | 952880 | 240104 | 74% |
  | bitcoin-wallet | 7992888 | 5365984 | 32% |
  | bitcoind | 13421336 | 11868592 | 12% |
  | bitcoin-qt | 37680496 | 31640976 | 16% |

ACKs for top commit:
  laanwj:
    Tested ACK 0947726

Tree-SHA512: 6b8483ea490e57a153105ad8c38b25fb1af5d55b1af22db398c7c2573612aaf71b4d2b4cf09c18fd6331b1358dba01641eeaa03e5018a925392e1937118d984a
@lyricidal lyricidal mentioned this pull request Mar 30, 2023
Merged
@bitcoin bitcoin locked and limited conversation to collaborators Jun 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@prusnak prusnak prusnak approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Build system
Projects
None yet
Milestone
24.0
Development

Successfully merging this pull request may close these issues.
6 participants
@fanquake @DrahtBot @jarolrod @hebasto @laanwj @prusnak