dblaikie added a comment.
@MaskRay I think this change is probably the best point of
comparison/demonstration of this patch direction (taking some minor liberties
with this patch to simplify things somewhat in ways that have already been
discussed/seem quite reasonable - specifically having `getCompresisonSpec`
return a reference and the enum having either no "unknown" value, or
`getCompressionSpec` asserting on the unknown value):
// removing the hardcoded zlib compression kind parameter in favor of what
the code would look like in the near future once it's parameterized on a
compression kind
CompressionImplementation *Compressor =
getCompressionSpec(CompressionAlgorithmKind)->Implementation;
bool DoCompression = Compress && DoInstrProfNameCompression && Compressor;
if (DoCompression) {
Compressor->compress(arrayRefFromStringRef(FilenamesStr),
CompressedStr,
Compressor->BestSizeLevel);
}
I think a reasonable speculation about what the alternative would look like in
the a non-OO design would be something like:
bool DoCompression = Compress && DoInstrProfNameCompression &&
isAvailable(CompressionAlgorithmKind);
if (DoCompression) {
compress(CompressionAlgorithmKind, arrayRefFromStringRef(FilenamesStr),
CompressedStr, getBestSizeLevel(CompressionAlgorithmKind));
And having these three correlated calls (`isAvailable`, `compress`, and
`getBestSizeLevel`) all made independently/have to have the same compression
algorithm kind passed to them independently seems more error prone & redundant
(not in an actual execution/runtime efficiency perspective - I don't think any
of these different designs would have materially different runtime performance
- but in terms of "duplicate work, and work that could diverge/become
inconsistent" - essentially the duplicate work/repeated switch statements, one
in each `compression::*` generic entry point seems like a code smell to me that
points towards a design that doesn't have that duplication) rather than tying
these calls together and making the lookup happen once and then using the
features after that. Also exposing the compression algorithm along with the
availability in one operation (not exposing compress/decompress/size APIs when
the algorithm isn't available) seems to have similar benefits.
I agree it's somewhat overkill for two types of compression, but I don't think
it's so burdensome as to be avoided either. I probably wouldn't've put quite
this much consideration into the design if I were doing it myself amongst other
things, but I appreciate that @ckissane has & I'm pretty happy with where this
is now.
A few things still leftover that I think I've mentioned to @ckissane offline:
- getCompressionSpec returning by reference
- more incremental patches - implementing the generic API in terms of the old
one (or keeping the old one as a wrapper around the generic one) with at most
one or two uses of the API (if any) in the first commit - then subsequent API
migrations in future commits, eventually removing the old API once the
migration is complete
- having only the `getCompressionSpec(CompressionKind)` and dropping the
version that takes uint8_t - callers should handle mapping from whatever
domain-specific format/representation they have into the CompressionKind enum,
there's no need for the raw version here
- Probably removing the 'unknown' CompressionKind - if a caller needs to handle
having "maybe a compression kind, or none" they can wrap CompressionKind in
Optional themselves and handle not looking up the algorithm if no compression
kind is desired/requested
================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:495
return collectPGOFuncNameStrings(
- NameStrs, compression::zlib::isAvailable() && doCompression, Result);
+ NameStrs, doCompression ? OptionalCompressionScheme : nullptr, Result);
}
----------------
ckissane wrote:
> dblaikie wrote:
> > looks like this could be changed to pass the implementation, without the
> > spec? (the caller doesn't need/use the spec)
> `(the caller doesn't need/use the spec)`...
> It definitely might in the future because it writes a header, that (in the
> conceivably near future) (like one of my follow up patches) could contain the
> uint 8 value of the compression type from the Spec, and 0 if uncompressed.
That should/can wait for the future change.
& I think at that point we could get into what the right design might be - and
I was thinking maybe the right design might be for the
CompressionImplementation to have a pointer back to its spec. But in general,
lets defer that design decision for later - it's not too expensive to change
what the parameter types are once the data is needed later.
(again, this might be even simpler addressed by not including this change at
all up-front - keeping the old API and leaving these callers until you're
patching the callers to add the new functionality of being able to choose
between Zlib and Zstd - rather than, for now, plumbing in the new API without
any change in functionality - the plumbing + functionality together might be
easire to understand the motivation for and review, and doing it separately
from most of the generic design work will isolate the usage design discussion
so each piece can move forward separately, for the most part)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131992/new/
https://reviews.llvm.org/D131992
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
