Skip to content

[memprof] Move Frame::hash and hashCallStack to IndexedMemProfData (NFC) #120365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

kazutakahirata
Copy link
Contributor

Now that IndexedMemProfData::{addFrame,addCallStack} are the only
callers of Frame::hash and hashCallStack, respectively, this patch
moves those functions into IndexedMemProfData and makes them private.
With this patch, we can obtain FrameId and CallStackId only through
addFrame and addCallStack, respectively.

Now that IndexedMemProfData::{addFrame,addCallStack} are the only
callers of Frame::hash and hashCallStack, respectively, this patch
moves those functions into IndexedMemProfData and makes them private.
With this patch, we can obtain FrameId and CallStackId only through
addFrame and addCallStack, respectively.
@llvmbot llvmbot added the PGO Profile Guided Optimizations label Dec 18, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 18, 2024

@llvm/pr-subscribers-pgo

Author: Kazu Hirata (kazutakahirata)

Changes

Now that IndexedMemProfData::{addFrame,addCallStack} are the only
callers of Frame::hash and hashCallStack, respectively, this patch
moves those functions into IndexedMemProfData and makes them private.
With this patch, we can obtain FrameId and CallStackId only through
addFrame and addCallStack, respectively.


Full diff: https://github.com/llvm/llvm-project/pull/120365.diff

2 Files Affected:

  • (modified) llvm/include/llvm/ProfileData/MemProf.h (+20-19)
  • (modified) llvm/lib/ProfileData/MemProf.cpp (+1-1)
diff --git a/llvm/include/llvm/ProfileData/MemProf.h b/llvm/include/llvm/ProfileData/MemProf.h
index ef96b74c9d400c..883e24d7186152 100644
--- a/llvm/include/llvm/ProfileData/MemProf.h
+++ b/llvm/include/llvm/ProfileData/MemProf.h
@@ -323,21 +323,6 @@ struct Frame {
        << "        Column: " << Column << "\n"
        << "        Inline: " << IsInlineFrame << "\n";
   }
-
-  // Return a hash value based on the contents of the frame. Here we use a
-  // cryptographic hash function to minimize the chance of hash collisions.  We
-  // do persist FrameIds as part of memprof formats up to Version 2, inclusive.
-  // However, the deserializer never calls this function; it uses FrameIds
-  // merely as keys to look up Frames proper.
-  inline FrameId hash() const {
-    llvm::HashBuilder<llvm::TruncatedBLAKE3<8>, llvm::endianness::little>
-        HashBuilder;
-    HashBuilder.add(Function, LineOffset, Column, IsInlineFrame);
-    llvm::BLAKE3Result<8> Hash = HashBuilder.final();
-    FrameId Id;
-    std::memcpy(&Id, Hash.data(), sizeof(Hash));
-    return Id;
-  }
 };
 
 // A type representing the index into the table of call stacks.
@@ -775,9 +760,6 @@ class CallStackLookupTrait {
   }
 };
 
-// Compute a CallStackId for a given call stack.
-CallStackId hashCallStack(ArrayRef<FrameId> CS);
-
 namespace detail {
 // "Dereference" the iterator from DenseMap or OnDiskChainedHashTable.  We have
 // to do so in one of two different ways depending on the type of the hash
@@ -1011,7 +993,7 @@ struct IndexedMemProfData {
   llvm::MapVector<CallStackId, llvm::SmallVector<FrameId>> CallStacks;
 
   FrameId addFrame(const Frame &F) {
-    const FrameId Id = F.hash();
+    const FrameId Id = hashFrame(F);
     Frames.try_emplace(Id, F);
     return Id;
   }
@@ -1027,6 +1009,25 @@ struct IndexedMemProfData {
     CallStacks.try_emplace(CSId, std::move(CS));
     return CSId;
   }
+
+private:
+  // Return a hash value based on the contents of the frame. Here we use a
+  // cryptographic hash function to minimize the chance of hash collisions.  We
+  // do persist FrameIds as part of memprof formats up to Version 2, inclusive.
+  // However, the deserializer never calls this function; it uses FrameIds
+  // merely as keys to look up Frames proper.
+  FrameId hashFrame(const Frame &F) const {
+    llvm::HashBuilder<llvm::TruncatedBLAKE3<8>, llvm::endianness::little>
+        HashBuilder;
+    HashBuilder.add(F.Function, F.LineOffset, F.Column, F.IsInlineFrame);
+    llvm::BLAKE3Result<8> Hash = HashBuilder.final();
+    FrameId Id;
+    std::memcpy(&Id, Hash.data(), sizeof(Hash));
+    return Id;
+  }
+
+  // Compute a CallStackId for a given call stack.
+  CallStackId hashCallStack(ArrayRef<FrameId> CS) const;
 };
 
 struct FrameStat {
diff --git a/llvm/lib/ProfileData/MemProf.cpp b/llvm/lib/ProfileData/MemProf.cpp
index 1c240c3858cc76..15ab44934bbf0b 100644
--- a/llvm/lib/ProfileData/MemProf.cpp
+++ b/llvm/lib/ProfileData/MemProf.cpp
@@ -290,7 +290,7 @@ Expected<MemProfSchema> readMemProfSchema(const unsigned char *&Buffer) {
   return Result;
 }
 
-CallStackId hashCallStack(ArrayRef<FrameId> CS) {
+CallStackId IndexedMemProfData::hashCallStack(ArrayRef<FrameId> CS) const {
   llvm::HashBuilder<llvm::TruncatedBLAKE3<8>, llvm::endianness::little>
       HashBuilder;
   for (FrameId F : CS)

@kazutakahirata kazutakahirata merged commit 6fb967e into llvm:main Dec 18, 2024
10 checks passed
@kazutakahirata kazutakahirata deleted the memprof_cleanup_move_hash_functions branch December 18, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PGO Profile Guided Optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants