[PATCH] D45793: [XRay][compiler-rt] - Dedupe xray event type strings from different addresses.

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: lib/xray/xray_interface.cc:71
+// Keep track of unique strings registered as type ids.
+::__xray::UniqueStringTable *const TypeEventTable = new UniqueStringTable();
 
----------------
Couple of things:

- I don't think you need to use the fully qualified name for the type here, we're already in the `__xray` namespace.

- I would have thought we can initialise this lazily, on the first call to the type registration function.


================
Comment at: lib/xray/xray_unique_string_table.cc:21-24
+using __sanitizer::internal_strncmp;
+using __sanitizer::SpinMutex;
+using __sanitizer::SpinMutexLock;
+using __sanitizer::Vector;
----------------
I suspect you may not actually need these, if you're already in the `__xray` namespace.


================
Comment at: lib/xray/xray_unique_string_table.cc:38
+// even though the intended use cases are not high cardinality.
+uint32_t Murmur3_32(const char *key, std::size_t length) {
+  static constexpr uint32_t seed = 0x31415926;
----------------
Do we not have this yet in the sanitizer_common/... library? And do we actually need this?


================
Comment at: lib/xray/xray_unique_string_table.cc:99
+
+UniqueStringTable::UniqueStringTable() : impl_(new UniqueStringTableImpl()) {}
+
----------------
Consider using `__sanitizer::InternalAlloc(...)` then placement-new. That's a bit more friendly than calling `new` directly and relying on the implementation's allocator, which may not be the same as the allocator being used by the sanitizer libraries.


================
Comment at: lib/xray/xray_unique_string_table.h:29
+  UniqueStringTable();
+  uint16_t AddOrLookupValue(const char *const content, std::size_t length);
+  // Meant to be constructed once as a static storage duration.
----------------
Probably use the sanitizer-local typedefs for some of these values -- say `u16` instead of `uint16_t`, etc.


================
Comment at: lib/xray/xray_unique_string_table.h:36
+private:
+  UniqueStringTableImpl *impl_;
+};
----------------
Do you really need to use the pimpl idiom?


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D45793