Skip to content

[memprof] Compare Frames instead of FrameIds in a unit test #119111

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

When we call IndexedMemProfRecord::toMemProfRecord, we care about
getting the original (that is, non-indexed) MemProfRecord back, so we
should just verify that, not the hash values, which are
intermediaries.

There is a remote possibility of hash collisions where call stack
{F1, F2} might come back as {F1, F1} if F1.hash() == F2.hash() for
example. However, since FrameId uses BLAKE, the hash values should be
consistent across architectures. That is, if this test case works on
one architecture, it should work on others as well.

When we call IndexedMemProfRecord::toMemProfRecord, we care about
getting the original (that is, non-indexed) MemProfRecord back, so we
should just verify that, not the hash values, which are
intermediaries.

There is a remote possibility of hash collisions where call stack
{F1, F2} might come back as {F1, F1} if F1.hash() == F2.hash() for
example.  However, since FrameId uses BLAKE, the hash values should be
consistent across architectures.  That is, if this test case works on
one architecture, it should work on others as well.
@llvmbot llvmbot added the PGO Profile Guided Optimizations label Dec 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 8, 2024

@llvm/pr-subscribers-pgo

Author: Kazu Hirata (kazutakahirata)

Changes

When we call IndexedMemProfRecord::toMemProfRecord, we care about
getting the original (that is, non-indexed) MemProfRecord back, so we
should just verify that, not the hash values, which are
intermediaries.

There is a remote possibility of hash collisions where call stack
{F1, F2} might come back as {F1, F1} if F1.hash() == F2.hash() for
example. However, since FrameId uses BLAKE, the hash values should be
consistent across architectures. That is, if this test case works on
one architecture, it should work on others as well.


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

1 Files Affected:

  • (modified) llvm/unittests/ProfileData/MemProfTest.cpp (+4-13)
diff --git a/llvm/unittests/ProfileData/MemProfTest.cpp b/llvm/unittests/ProfileData/MemProfTest.cpp
index 5e24579baccd37..55feb1c3f60b45 100644
--- a/llvm/unittests/ProfileData/MemProfTest.cpp
+++ b/llvm/unittests/ProfileData/MemProfTest.cpp
@@ -534,19 +534,10 @@ TEST(MemProf, IndexedMemProfRecordToMemProfRecord) {
 
   // Verify the contents of Record.
   ASSERT_THAT(Record.AllocSites, SizeIs(2));
-  ASSERT_THAT(Record.AllocSites[0].CallStack, SizeIs(2));
-  EXPECT_EQ(Record.AllocSites[0].CallStack[0].hash(), F1.hash());
-  EXPECT_EQ(Record.AllocSites[0].CallStack[1].hash(), F2.hash());
-  ASSERT_THAT(Record.AllocSites[1].CallStack, SizeIs(2));
-  EXPECT_EQ(Record.AllocSites[1].CallStack[0].hash(), F1.hash());
-  EXPECT_EQ(Record.AllocSites[1].CallStack[1].hash(), F3.hash());
-  ASSERT_THAT(Record.CallSites, SizeIs(2));
-  ASSERT_THAT(Record.CallSites[0], SizeIs(2));
-  EXPECT_EQ(Record.CallSites[0][0].hash(), F2.hash());
-  EXPECT_EQ(Record.CallSites[0][1].hash(), F3.hash());
-  ASSERT_THAT(Record.CallSites[1], SizeIs(2));
-  EXPECT_EQ(Record.CallSites[1][0].hash(), F2.hash());
-  EXPECT_EQ(Record.CallSites[1][1].hash(), F4.hash());
+  EXPECT_THAT(Record.AllocSites[0].CallStack, ElementsAre(F1, F2));
+  EXPECT_THAT(Record.AllocSites[1].CallStack, ElementsAre(F1, F3));
+  EXPECT_THAT(Record.CallSites,
+              ElementsAre(ElementsAre(F2, F3), ElementsAre(F2, F4)));
 }
 
 // Populate those fields returned by getHotColdSchema.

@kazutakahirata kazutakahirata merged commit 6c062af into llvm:main Dec 8, 2024
10 checks passed
@kazutakahirata kazutakahirata deleted the memprof_cleanup_unittest_nohash branch December 8, 2024 16:33
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