Page MenuHomePhabricator

[LLD][ELF] - Make compression level be dependent on -On.
ClosedPublic

Authored by grimar on Nov 25 2019, 1:41 AM.

Details

Summary

Currently LLD always use zlib compression level 6.
This patch changes it to use 1 for -O0, -O1 and 6 for -O2.

It fixes https://bugs.llvm.org/show_bug.cgi?id=44089.

There was also a thread in llvm-dev on this topic:
https://lists.llvm.org/pipermail/llvm-dev/2018-August/125020.html

Here is a table with results of building clang mentioned there:

Level   Time            Size
0       0m17.128s       2045081496   Z_NO_COMPRESSION
1       0m31.471s       922618584    Z_BEST_SPEED
2       0m32.659s       903642376
3       0m36.749s       890805856
4       0m41.532s       876697184
5       0m48.383s       862778576
6       1m3.176s        855251640    Z_DEFAULT_COMPRESSION
7       1m15.335s       853755920
8       2m0.561s        852497560
9       2m33.972s       852397408    Z_BEST_COMPRESSION

It shows that it is probably not reasonable to use values greater than 6.

Diff Detail

Event Timeline

grimar created this revision.Nov 25 2019, 1:41 AM
Herald added a project: Restricted Project. · View Herald Transcript
grimar updated this revision to Diff 230848.Nov 25 2019, 1:44 AM
  • Rename test case: *.s -> *.test
peter.smith accepted this revision.Nov 25 2019, 4:11 AM
peter.smith added a subscriber: peter.smith.

Looks good to me, looks like it matches the discussion in https://bugs.llvm.org/show_bug.cgi?id=44089 will be worth waiting for US timezone based reviewers to comment.

This revision is now accepted and ready to land.Nov 25 2019, 4:11 AM
MaskRay accepted this revision.Nov 25 2019, 9:45 AM
MaskRay added inline comments.
lld/test/ELF/compressed-debug-level.test
6

For long linker options, prefer -- to - (with a few exceptions like -Ttext)

We can probably use cmp %t.default %t.O1 for this test.

Probably attach the table in the commit message:

Level   Time            Size
0       0m17.128s       2045081496   Z_NO_COMPRESSION
1       0m31.471s       922618584    Z_BEST_SPEED
2       0m32.659s       903642376
3       0m36.749s       890805856
4       0m41.532s       876697184
5       0m48.383s       862778576
6       1m3.176s        855251640    Z_DEFAULT_COMPRESSION
7       1m15.335s       853755920
8       2m0.561s        852497560
9       2m33.972s       852397408    Z_BEST_COMPRESSION

https://bugs.llvm.org/show_bug.cgi?id=44089

So, I think we should use level 1 for the default and level 6 for -O2, and I don't think we need a new option to specify some other value as a compression level.

-O0 => level 1
-O1 => level 1
-O2 => level 6

makes sense.

We use -Wl,--compress-debug-sections=zlib and default -Wl,-O1 for asan builds, so some users may observe size increases (due to: zlib level 6 -> zlib level 1). I will try accommodating the targets not to rely on the size, but I may request for a new option for compression level (@ruiu) if fixing them is not feasible. (I genuinely want to keep the configuration simple, so not having an additional option is also my preference.)

ruiu accepted this revision.Nov 26 2019, 12:02 AM

LGTM

ruiu added inline comments.Nov 26 2019, 12:20 AM
lld/ELF/OutputSections.cpp
273

It's worth adding a comment here to explain why we chose 6 and 1 as optimization levels. We chose 1 as the default compression level because it is the fastest. If -O2 is given, we use level 6 to compress debug info more by ~15%. We found that level 7 to 9 doesn't make much difference (~1% more compression) while they take significant amount of time (~2x), so level 6 seems enough.

grimar edited the summary of this revision. (Show Details)Nov 26 2019, 12:37 AM
This revision was automatically updated to reflect the committed changes.
grimar marked 3 inline comments as done.

I've applied all the changes suggested in comments before committing. Thanks!

smeenai added inline comments.
lld/test/ELF/compressed-debug-level.test
23

The size appears to depend on the zlib version. For example, I'm running with zlib 1.2.8, which manages to compress these contents to the same size with both level 1 and 6. Is there any way to make this agnostic to zlib version?

MaskRay added inline comments.Nov 26 2019, 4:36 PM
lld/test/ELF/compressed-debug-level.test
23

I run zlib 1.2.11 and it works for me. Can you alter the input a bit to make the length different for level 1 and level 6, for zlib 1.2.8?

smeenai added inline comments.Nov 26 2019, 5:19 PM
lld/test/ELF/compressed-debug-level.test
23
grimar marked an inline comment as done.Nov 27 2019, 12:38 AM
grimar added inline comments.
lld/test/ELF/compressed-debug-level.test
23

I fixed it with 75fd939bb917e8f843395684a2970d86bc0199c0.

Thanks! FTR, I used zlib 1.2.3 under windows when wrote this. Now I wonder how much brittle to check the size produced.
Seems it is not safe enough?
I think what we can do instead is to print llvm-readelf output to files for pairs [-O0, -O2], [-O1, -O2] etc and then do something
like

# CHECK: [ 1] .debug_info PROGBITS 00000000 000094 [[SIZE:*]]
# CHECK-NOT: [ 1] .debug_info PROGBITS 00000000 000094 [[SIZE]]

i.e. this should check that sized are just different. Should we?

smeenai added inline comments.Nov 27 2019, 9:06 AM
lld/test/ELF/compressed-debug-level.test
23

The issue I ran into with the original test case was that the compressed data had the same size with levels 1 and 6, so just verifying the sizes were different wouldn't help.

I agree that checking sizes isn't super robust, but it seems to work okay for now with the updated test case?

MaskRay added inline comments.Nov 27 2019, 9:28 AM
lld/test/ELF/compressed-debug-level.test
23

We could use not cmp %t.0 %t.2, but since it seems to work for zlib 1.2.3, 1.2.8, 1.2.11, probably not worth updating it again.