[memprof] Introduce readMemProf (NFC)

[kazutakahirata]

This patch refactors the deserilization of the MemProf data to a
switch statement style.

switch (Version) {
case Version0:
return ...;
case Version1:
return ...;
}

Since memprof::Version0 doesn't really have a version number in the
header, the switch statement only has one case for now.

[teresajohnson]

We end up with a ton of duplicated code in the modified approach - is there enough value in doing this?

[snehasish]

@teresajohnson I suggested this approach. I think it's simpler in the short term as we iterate quickly on new versions. I'm thinking about a case where we have 3-4 different versions which reuse the same logic with multiple conditions. I think it will be hard to reason about, error prone and difficult to remove. Once the format stabilizes we can refactor the implementation to keep the versions we care about. What do you think?

[snehasish]

Maybe just use an if-condition until we have additional cases?

[teresajohnson]

@teresajohnson I suggested this approach. I think it's simpler in the short term as we iterate quickly on new versions. I'm thinking about a case where we have 3-4 different versions which reuse the same logic with multiple conditions. I think it will be hard to reason about, error prone and difficult to remove. Once the format stabilizes we can refactor the implementation to keep the versions we care about. What do you think?

Since the only current difference is in the way RecordTableOffset is set, perhaps all of the common stuff can be refactored out and each version do something like:

readMemProfVx() {
   const uint64_t RecordTableOffset = (version specific setting);
   return readMemProfImpl(RecordTableOffset);
}

I dislike having so much identical code because it becomes more difficult to reason about and update consistently.

[snehasish]

difficult to reason about and update consistently.

The same rationale motivated my suggestion. By separating out the implementation for each version, we don't have to update them at all and simply remove them as they age out. Wouldn't that be easier to reason about as opposed to more complex control flow?

[teresajohnson]

difficult to reason about and update consistently.

The same rationale motivated my suggestion. By separating out the implementation for each version, we don't have to update them at all and simply remove them as they age out. Wouldn't that be easier to reason about as opposed to more complex control flow?

I guess it depends how how much we expect to diverge in future versions. If you think this is going to be easier I am not opposed.

[snehasish]

I guess it depends how how much we expect to diverge in future versions.

Yes, I expect divergence with the introduction of new sections for the summary and callstack hashtable. Internal format changes to the meminfoblock might be less intrusive and be better for refactoring and we can revisit at that time. I will let @kazutakahirata decide.

(accidentally edited the previous comment instead of posting a new one)

[kazutakahirata]

Thank you for comments. I'm planning to add at least one more item to the indexed MemProf format. Let me revisit this issue at that time.