This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Specify -section argument for objdump calls in testcases.
AbandonedPublic

Authored by grimar on Dec 24 2016, 12:37 AM.

Details

Summary

My motivation for doing that was to check if that can
speed up testcases execution time a little. For example running check-lld
under windows takes about 21 second for me.

Unfortunately this change had no effect on testcase execution time at all.
Though It still looks a reasonable cleanup for testcases output.

Diff Detail

Event Timeline

grimar updated this revision to Diff 82437.Dec 24 2016, 12:37 AM
grimar retitled this revision from to [ELF] - Specify -section argument for objdump calls in testcases..
grimar updated this object.
grimar added reviewers: ruiu, rafael, davide.
grimar added subscribers: llvm-commits, grimar, evgeny777.
davide edited edge metadata.Dec 24 2016, 4:00 AM

I don't quite understand the rationale behind this patch. You tried something to speed up which didn't work, so maybe you want to try profiling llvm-objdump and see where the time is spent and try to optimize that instead of landing this patch? Also, did you profile the tests execution and actually found out objdump is the real bottleneck? Keep in mind running LLVM tests on Windows is notoriously slower than on *NIX.

I don't quite understand the rationale behind this patch. You tried something to speed up which didn't work, so maybe you want to try profiling llvm-objdump and see where the time is spent and try to optimize that instead of landing this patch? Also, did you profile the tests execution and actually found out objdump is the real bottleneck? Keep in mind running LLVM tests on Windows is notoriously slower than on *NIX.

I am mostly suggest this for consistency. We already use -section argument in many tests. That also has a positive effect when need to debug testcase for some reason. I mean reducing output of sections content is still usefull to have.

I did not try to profile the tests yet, just checked that idea as supposed that probably can change something.

I don't quite understand the rationale behind this patch. You tried something to speed up which didn't work, so maybe you want to try profiling llvm-objdump and see where the time is spent and try to optimize that instead of landing this patch? Also, did you profile the tests execution and actually found out objdump is the real bottleneck? Keep in mind running LLVM tests on Windows is notoriously slower than on *NIX.

I am mostly suggest this for consistency. We already use -section argument in many tests. That also has a positive effect when need to debug testcase for some reason. I mean reducing output of sections content is still usefull to have.

Why is reducing the output of objdump useful? If that would have an impact on performance, I'd agree with you, but you realized it hasn't, so, I don't understand what's the real motivation behind this change.
As a side note, this makes slightly more annoying updating tests. If you rename a section, you need to modify two places, and if you're adding a section which you need to dump, then you need to change the linker invocation. Not the end of the world, but why?

ruiu edited edge metadata.Jan 3 2017, 4:02 PM

I think I agree with Davide. This patch might not necessarily be a bad change, but to me it seems to be adding extraneous command line options to so many command invocations. As the tests work without these command line options, I wouldn't add that based on Occum's razor-ish principle. I want to keep it simple.

grimar abandoned this revision.Jan 9 2017, 1:57 AM

As a side note, this makes slightly more annoying updating tests. If you rename a section, you need to modify two places, and if you're adding a section which you need to dump, then you need to change the linker invocation. Not the end of the world, but why?

We probably usually do not rename or add sections when updating tests. That happens sometimes, but more often case is updating checked values when test fails, in that case reducing of printed data can be useful. It is IMO a bit more convinent to see data of single section on output to find the difference with test than have data from many sections both used and unused in checks. I was sure we used -section=xxx in other tests because of that reason (to keep output minimized to help debugging testcases a bit).
At least that was my motivation in other patches where I added -section=xxx intitially.

Now it is not clear for me why we have this parameter in other tests then. May be it worst to remove it then ?

Since you and Rui are not fond of this patch, I am abandoning this diff.