[PATCH] D148868: [llvm-profdata] ProfileReader cleanup - preparation for MD5 refactoring

[William Junda Huang via Phabricator via llvm-commits]

huangjd added a comment.

In D148868#4297862 <https://reviews.llvm.org/D148868#4297862>, @wenlei wrote:

> Hoisting all the MD5 stuff out of compact binary format, and merge them with extended binary format makes sense. Initially compact binary format was the only one using MD5, but later on Wei added MD5 support in extended binary as well, which caused some of the duplication this patch is trying to address.
>
> However, from design POV, it's a good practice to keep the specific implementation down to leaf type as much as possible. Here MD5 only comes into play for extended binary and compact binary, so exposing that to all binary format is less than ideal. I think the problem is that compact binary and extended binary started out to be very different, but involved into something similar, so the type structure no longer reflects that commonality. Now in order to deduplicate code, you have to expose MD5 stuff to their lowest common ancestor, which is binary format.
>
> Actually in https://reviews.llvm.org/D76255, we talked about eventually remove compact binary. Can we simply remove compact binary now? Then we can keep MD5 stuff in extended binary still, and there will be no duplicates.
>
> cc @hoy @wlei

I would like to limit the scope of the refactoring not to include another major change, since other reviewers would like to go through small patches. The purpose of my series of refactoring is to change the data representation of profile map, using MD5 as the key, which could bring significant speedup to profile load time. Reducing overloaded functions with similar logic make the code less error prone when changing the data representation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148868