(flyby comment) is it a "perfscript" or a "perftrace" - or maybe "perfdata" rather.

Also, probably "perf tool" not "perf script" in the description.

Path to profiled binary?

Maybe use BinaryTable.insert instead of BinaryTable.find above and use the return value to check whether the binary has been loaded before, then you don't have to search the table another time here.

Here it is may or may not?

It is a map from address to ProfiledBinary, how about using AddressBinaryMap?

Here BinaryAddrMap needs to erase an entry before inserting a new one. I guess it is to support the case that a load image is unloaded and then reloaded at a different place. If that is correct, please add some comment to make it clear.

Nit: to help maintainability down the road, perhaps ensure primitive fields are initialized here - I realize they are init-ed in the ctor, but it's even easier to maintain things when def and init on the same line. Less typing for the ctor, too.

best to initialize struct fields to avoid undefined values.

should this rather be Buffer->getBufferSize() > static_cast<size_t>(std::numeric_limits<uint32_t>::max())?

could P+1 be out of bounds?

should this set IsLoaded to false?

also, why not assert(!IsLoaded); or rename to "ensureLoaded";

or - is there value in a ProfiledBinary that's not loadable? If not, how about:

• private ctor
• fields are const; no need for IsLoaded
• public static factory method returning a std::unique_ptr<ProfildBinary> that's null if loading fails.

that way, a reader knows they don't need to bother worrying about ProfiledBinaries that don't work - simpler overall state. wdyt?

consider using an enum like:

enum EventIndex {

WholeLine = 0,
PID = 1,
BaseAddress = 2... (etc)

}

so then dereferencing Fields is more readable.

I guess this can happen because the Event could reference other binaries than the interesting ones, correct? Could you add a comment here about this - i.e. that the event is intentionally dropped here.

why call load - why not just fetch the ProfiledBinary entry? Unless I'm missing something, the expected behavior is that first the user-provided binaries are loaded; and if that goes well, then the events for them are loaded. So at this point the ProfiledBinary should be there?

perhaps instead of 'run' something more specific, like "loadBinariesAndEvents".

probably can be done later just fine, but should the main be in its own file, so then this becomes a reusable library?

One thought on using MemoryBuffer::getFileOrSTDIN (or other similar MemroyBuffer based file reading):

• there could be scalability issues when processing huge (10G+) files, because the way MemoryBuffer::getFileOrSTDIN reads files is it reads the whole content into memory (or do a mmap) in oneshot, this imposes a huge burden on memory io.

• for perf script output parsing, it is mostly line based - each line is processed and discarded, this suggests that a stream based processing is more suitable and could be much more efficient. A straightforward way (with lower level io operations) could be like this: std::ifstream fin(perf_script_filename); if (!fin.good()) { /* error */ } for (std::string line; std::getline(input, line); ) { parseEvent(line); }

• this way, the memory consumption is almost constant regardless of the inpurt perf script file.

What do you think?

const line_iterator& ?

What about we pass in BinaryFilenames and PerfTraceFilenames as parameters to this function (or to ctor), instead of letting PerfReader coupled with command-line options directly. Then perhaps name it readFromInput(BinaryFilenames, PerfTraceFilenames).