This is an archive of the discontinued LLVM Phabricator instance.

Optimize objdump -objc-meta-data
ClosedPublic

Authored by kastiglione on Dec 20 2016, 9:52 AM.

Details

Reviewers
compnerd
enderby
Summary

Running a Debug build of objdump -objc-meta-data with a large Mach-O file is currently unnecessarily slow.

With some local test input, this change reduces the run time from 75-85s down to 15-20s.

The two changes are:

  1. Assert on pointer equality not array equality
  2. Replace vector<pair<address, symbol>> with DenseMap<address, symbol>

Event Timeline

kastiglione retitled this revision from to Optimize objdump -objc-meta-data.
kastiglione updated this object.
kastiglione added a reviewer: compnerd.
kastiglione added a subscriber: llvm-commits.
compnerd edited edge metadata.Jan 2 2017, 2:21 PM

I dont understand the changes to the assertion, could you please explain that?

tools/llvm-objdump/MachODump.cpp
1796

Please use a std::unique_ptr to avoid the leak.

I dont understand the changes to the assertion, could you please explain that?

Checking pointer equality first, before falling back on a more costly ArrayRef content equality.

Better answer:

The reason the assertion is so costly is that each iteration (of bind table entries) calls MachOBindEntry::operator==, which in turn does a member-wise equality check of the full array of bind opcodes. For a large Mach-O file, this can become time consuming and can be mitigated by a pointer equality check first.

kastiglione edited edge metadata.

Use unique_ptr

compnerd requested changes to this revision.Jan 3 2017, 11:31 AM
compnerd edited edge metadata.
compnerd added inline comments.
lib/Object/MachOObjectFile.cpp
3079

These changes are technically introducing UB. Comparing iterators across containers is UB. So, we need a member-wise comparison. You can put this under expensive checks if you want though.

This revision now requires changes to proceed.Jan 3 2017, 11:31 AM
kastiglione edited edge metadata.

Compare .data() instead of .begin()

kastiglione marked 2 inline comments as done.Jan 3 2017, 11:39 AM
compnerd added inline comments.Jan 3 2017, 11:54 AM
lib/Object/MachOObjectFile.cpp
2827

Why not just change this to:

assert(Opcodes.data() == Other.Opcodes.data() && Opcodes.size() == Other.Opcodes.size() && "comparing differing opcodes");
kastiglione added inline comments.Jan 3 2017, 1:20 PM
lib/Object/MachOObjectFile.cpp
2827

The current assertion has value semantics, this change would switch the assertion to allow reference equality only. I'm not sure why the assertion is currently constructed the way it is. I don't think any of llvm would break, but some external code could in theory. If you think it's best to change, I'll go with it.

compnerd added inline comments.Jan 3 2017, 3:20 PM
lib/Object/MachOObjectFile.cpp
2827

Except that after your change, you are short-circuiting the check to pointer equality rather than value equality. So, changing it entirely to pointer equality might be the best approach. Are expensive asserts enabled by default? We could just demote this to an expensive check and leave it as a full value comparison as well.

kastiglione updated this revision to Diff 82965.Jan 3 2017, 3:57 PM
kastiglione marked an inline comment as done.
kastiglione edited edge metadata.

Make the assertion depend on EXPENSIVE_CHECKS

compnerd accepted this revision.Jan 3 2017, 6:31 PM
compnerd edited edge metadata.
This revision is now accepted and ready to land.Jan 3 2017, 6:31 PM

Thanks @compnerd. If you are able to commit this too, I'd appreciate it.

compnerd closed this revision.Jan 8 2017, 11:31 AM

SVN r291398.