enable lto, panic = "abort" in release build

[ordian]

• shaves 11 MB off binary size (from 44MB to 33MB for RUSTFLAGS=" -C link-arg=-s" cargo build --release, -8MB thanks to LTO)
• run time should be faster, but I haven't measured

Last time we disabled panic = "abort" in #8983 (cc @sorpaas) due to failing build.

[niklasad1]

This is awesome but it increases the build-time significantly (roughly 2x) for me but definitely worth the effort, however I'm not really aware of the exact differences between panic=unwind and panic=abort I'll leave it to the pro's

# this branch
➜  parity-ethereum git:(optimize-release-build) ✗ size --format=SysV ./target/release/parity
size --format=SysV ./target/release/parity
./target/release/parity  :
section                  size       addr
.interp                    28        736
.note.ABI-tag              32        764
.note.gnu.build-id         36        796
.gnu.hash                1004        832
.dynsym                  9744       1840
.dynstr                  9346      11584
.gnu.version              812      20930
.gnu.version_r            768      21744
.rela.dyn             1237392      22512
.rela.plt                4584    1259904
.init                      23    1265664
.plt                     3072    1265696
.plt.got                  504    1268768
.text                25521701    1269312
.fini                       9   26791016
.rodata               4913113   26791936
.eh_frame_hdr          168428   31705052
.eh_frame             1142048   31873480
.gcc_except_table       66692   33015528
.tdata                    818   33089920
.tbss                     680   33090752
.init_array              1064   33090752
.fini_array                 8   33091816
.data.rel.ro           980640   33091840
.dynamic                  624   34072480
.got                     5616   34073104
.data                   11408   34078720
.bss                    31744   34090144
.comment                   56          0
Total                34111994

# master
parity-ethereum git:(master) ✗ size --format=SysV ./target/release/parity
./target/release/parity  :
section                  size       addr
.interp                    28        736
.note.ABI-tag              32        764
.note.gnu.build-id         36        796
.gnu.hash                1004        832
.dynsym                  9960       1840
.dynstr                  9537      11800
.gnu.version              830      21338
.gnu.version_r            800      22168
.rela.dyn             1708824      22968
.rela.plt                4608    1731792
.init                      23    1736704
.plt                     3088    1736736
.plt.got                  504    1739824
.text                28453253    1740352
.fini                       9   30193608
.rodata               8373294   30195712
.eh_frame_hdr          525348   38569008
.eh_frame             3148480   39094360
.gcc_except_table     1451248   42242840
.tdata                    752   43701120
.tbss                     672   43701888
.init_array              1064   43701888
.fini_array                 8   43702952
.data.rel.ro          1221272   43702976
.dynamic                  624   44924248
.got                    49208   44924872
.data                   11466   44974080
.bss                    31760   44985568
.comment                   56          0
Total                45007788

[ordian]

This is awesome but it increases the build-time significantly (roughly 2x) for me

Don't we build --release only on releasable branches (stable and beta)?
Also, IIUC, profile.release shouldn't affect cargo test --release (should be covered by profile.bench).
EDIT: I might be wrong

The release profile, used for cargo build --release (and the dependencies
for cargo test --release, including the local library or binary)

[sorpaas]

Last time when we disabled abort, besides building fails, one of the other reason is that we got bug reports that some state trie items are missing, or blocks are missing, which triggers the client to panic. One of the probable reason is that the client doesn't shutdown correctly (through another panic, for example), and left in the middle of an import. In that case, we can be in a situation where we commit some metadata, but not actually committed the block itself, or we may in the middle of committing the state trie. Using rewind would potentially allows the shutdown to be happen more gracefully in case a panic happened (while the panicking thread would fail, other threads can receive the shutdown signal and do stuff if needed), but I also haven't really done deep analysis of how much benefits we would gain with rewind.

We also discussed enabling lto last time. The reason why we didn't at that time was that devops worried that lto would make release build machine fail, because it can take a large amount of memory. But if devops confirm that this is fine now (if we have upgraded our machines!), then this shouldn't be an issue any more.

[ordian]

Regarding LTO:

./test.sh on CI on this branch:

7326.09user 2098.40system 14:40.37elapsed 1070%CPU 5.8 GB mem

./test.sh on CI for other PRs:

4002.26user 536.6.9system 11:39.47elapsed 648%CPU 4.3 GB mem
4006.64user 1955.58system 15:45.66elapsed 630%CPU 5.4 GB mem link
4752.03user 447.98system 18:36.40elapsed 465%CPU link

Looks like the overall time for building + running tests hasn't changed much. Max RSS does not seem to be too large.
Alternatively, we can backport this PR to beta and stable without merging into master.

Now on to panic = "abort":

@arkpar argues here that:

Database has to be resistant to power loss anyway.

But now, in addition to KeyValueDB, we have blooms-db, which is not power loss resistant. So that's a valid concern, I guess.

EDIT: OTOH, we're using parking_lot, which is not panic safe, so panic = "abort" could potentially make unclean shutdown situation better as well.

[arkpar]

When I last checked (around 2017) panic="abort" did have a substantial effect of performance. Block import was around 5-10% faster.
The fact that there are three databases now makes atomic updates much harder. It would have been much better to use column families instead, as these can be written with a single atomic transaction.
Even with panic="unwind"the three databases my go out of sync because of the power loss or process termination. There needs to be some additional code that would detect this by e.g. comparing best block number or hash stored in all of the databases. In case of mismatch it would either have to rewind the main database to an older block, or rebuild the missing traces.

Anyway, panic and abort in the middle of an import won't lead to main database corruption because transaction atomicity is guaranteed by rocksdb, and as far as I know all of the block import is done in a single transaction.

[ordian]

Ok, it seems like we all agree on the LTO change, if someone from devops can confirm it won't kill our machines (@gabreal ?).

Anyway, panic and abort in the middle of an import won't lead to main database corruption because transaction atomicity is guaranteed by rocksdb, and as far as I know all of the block import is done in a single transaction.

This matches what @debris said here:

Existence of blooms is not consensus critical. If one of the writes fail (or partially fail), the particular block will be reimported on the next launch of parity and the database will overwrite corrupted data

If we agree on panic = "abort", should #8999 be reverted?
If we don't, I can revert that change, or, alternatively, port it only to beta, to see if it would lead to more bug reports. Any thoughts?

[gabreal]

As far as I can see, there should be no problem of memory anymore, but be aware that the test job run time increases approx about 7min (for this job there is only one sample). @TriplEight might also like to know.

[5chdn]

Don't we build --release only on releasable branches

tests are run in release mode afaik.

[TriplEight]

Thanks for mentioning @gabreal .

tests are run in release mode afaik.

Yes and here. And would be really good if we could avoid it to make this features work only for the release builds.

Currently merging this will slow down our already slow test pipelines.

Please read about LTO here, really a good read, and there are some benchmarks of different features.

Compilers build and optimize compilation units individually. LTO is a mechanism for taking those 
individually-optimized object files and finding more opportunities for optimization when they are 
considered as a group. This is great if you want to have the fastest possible program, but the price you 
pay are higher compilation times. In Rust, the compilation unit is the crate; if the heavy computations 
are all done in your crate, then maybe LTO isn’t necessary.

Regarding panic handling, panic = abort just does not include unwind libraries to the release, that's how it becomes lighter.

By removing support for unwinding, you'll get smaller binaries. You will lose the ability to catch panics. 

That way we are refusing bug stack traces from users.

[ordian]

Currently merging this will slow down our already slow test pipelines.

That's why I suggested to only backport it to stable and beta, w/o merging into master.

That way we are refusing bug stack traces from users.

catch panics != stack traces, we'll see stack traces

[TriplEight]

That way it makes sense @ordian .

[TriplEight]

Just had a talk with Pierre. He suggested that we should set the

[profile.test]
opt-level=0 # or 1
panic=unwind
lto=false

that way we can get rid of --release for all the tests, in general it should make tests faster.

[ordian]

in general it should make tests faster.

It would make tests faster to compile but slower to run, I think the reason we're using cargo test --release in the first place is because the overall (compile + run) time is faster than the one w/o --release, but it would be nice to see if it can be reduced with opt-level=1.

[TriplEight]

I'll try this opt-level=1 locally, will report

[gabreal]

I think the reason we're using cargo test --release in the first place is because the overall (compile + run) time is faster than the one w/o --release,

In fact I changed that a while ago to have the tests perform as close as possible as they are used in the wild.

[ordian]

I've split the change into two commits, the second commit also removes the std::process::abort() call from the panic hook.

My suggestion is to backport the first commit to stable and both to beta.

[TriplEight]

@gabreal @ordian a comparsion report: https://drive.google.com/open?id=159CBQGiKOPz9JBfda_n9bLFNF10qIif3Fyz5DoyWBjk

[dvdplm]

To my ears it sounds like we want lto = true for release builds and lto = false for tests (but otherwise keep the same optimisation level as release for tests).

Panic handling seems more contentious and it sounds like we need more testing and hand-wringing on that one.

@ordian can you PR those two changes separately? The diff here is a little confusing with all the Clique code in.

[ordian]

To my ears it sounds like we want lto = true for release builds and lto = false for tests (but otherwise keep the same optimisation level as release for tests).

I'm not sure how to enable it this way, since we run tests with --release flag if I'm not mistaken. So I suggested to not this PR into master, but rather backport commits into stable and beta. What do you think?

The diff here is a little confusing with all the Clique code in.

Yeah, that's due to force rebase on master.

[dvdplm]

since we run tests with --release flag

Forgive me, but I'm slow. Can't we stop running tests that way and instead switch to:

[profile.test]
# proper optimization levels to match --release here
lto = false

…i.e. switch CI to run cargo test instead of cargo test --release? I must be missing something!

[TriplEight]

@dvdplm yes, this thread is hard.

Can't we stop running tests that way and instead switch to

AFAIU we can do that.

[TriplEight]

@dvdplm, @ordian something went wrong, I suggest falling back.
Pipeline with this change: https://gitlab.parity.io/parity/parity-ethereum/pipelines/39507/builds
Any other pipeline: https://gitlab.parity.io/parity/parity-ethereum/pipelines/39408/builds
Check out the time it took to test, it's 48 vs 11 min!

[dvdplm]

@TriplEight the links are the same there, not sure where to look. Can fix? :)

[TriplEight]

@dvdplm I edited in the GitHub UI