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

[Keith via Phabricator via llvm-commits]

kpw added a comment.

Haven't made any changes yet. Thanks for the comments Dean.



================
Comment at: lib/xray/xray_interface.cc:71
+// Keep track of unique strings registered as type ids.
+::__xray::UniqueStringTable *const TypeEventTable = new UniqueStringTable();
 
----------------
dberris wrote:
> 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.
1. Good catch.
2. We can do it either way.


================
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;
----------------
dberris wrote:
> Do we not have this yet in the sanitizer_common/... library? And do we actually need this?
1. I saw a murmur 2 implementation, but it was part of an implementation file.
2. I started out wanting to build a hashmap without resizing. Essentially it would grow up until a load factor with something layers like BlockLink. Collision would cause array-wrapped lookahead, and it grows by adding more layers. You can find a value in lookups ~= to the number of Block layers, and it would never have to resize. I decided this patch was "good enough" though.


================
Comment at: lib/xray/xray_unique_string_table.cc:99
+
+UniqueStringTable::UniqueStringTable() : impl_(new UniqueStringTableImpl()) {}
+
----------------
dberris wrote:
> 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.
Cool. Didn't know about that method until recently.


================
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.
----------------
dberris wrote:
> Probably use the sanitizer-local typedefs for some of these values -- say `u16` instead of `uint16_t`, etc.
What's the difference between these? When and why should I prefer sanitizer types?


================
Comment at: lib/xray/xray_unique_string_table.h:36
+private:
+  UniqueStringTableImpl *impl_;
+};
----------------
dberris wrote:
> Do you really need to use the pimpl idiom?
No, but the class must have a known size, which will uglify the header.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D45793