[PATCH] D56325: Sort symbols in .bss by size.

Fri Jan 11 16:50:16 PST 2019

ruiu added a comment.

In D56325#1353178 <https://reviews.llvm.org/D56325#1353178>, @grimar wrote:

> In D56325#1353138 <https://reviews.llvm.org/D56325#1353138>, @ruiu wrote:
>
> > I don't think we need a detailed benchmark for other targets, as how programs use .bss (and in general other parts of data sections) doesn't depend too much on targets.
>
>
> Hard to say. I remember we had a patch that fixed inconsistency of handling .bss symbols (we had an issue in a case when application wanted to have the same order of symbols it would have if we would create them in a command line order, it had a relocation overflow because we create the .bss symbols first now). The patch was declined (I can find it if you want). So if this patch solves a local issue of low-end Android handsets only I really see no reason of why we want to change the behavior of LLD? It is the almost the same case that patch had. The difference is that now it is needed for Android and not for an unknown application. Until we have benchmarks showing it has a benefit for other platforms I am not convinced we should change the linker. It is up to you anyways, I am just saying my opinion about that change.

For the record, the reason why I'm in favor of this patch not because I'm working for Google; it is not correct to say that this patch is to fix a local issue. I've been trying to handle all contributors equally. As to this patch, I think it doesn't only improve Android but improve every target. Android Go team is working hard to reduce memory usage, and that's why they found this heuristics, and I added that to the comment because a concrete example is more reader-friendly than saying the same thing abstractly. But nothing special about Android Go.

That said, I can ask Vic to build other regular Linux applications with/without this patch to see if the same improvement (or at least no regression) can be observed. Vic, do you mind if I ask you to do that?

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56325/new/

https://reviews.llvm.org/D56325