[PATCH] RFC: fail-fast iterators for DenseMap

Chandler Carruth chandlerc at gmail.com
Fri Feb 27 01:38:48 PST 2015


================
Comment at: include/llvm/ADT/ADTModCountTracker.h:16
@@ +15,3 @@
+#ifndef LLVM_ADT_ITERATOR_EPOCH_H
+#define LLVM_ADT_ITERATOR_EPOCH_H
+
----------------
Heh. Pick a name and be consistent?

And please don't repeat 'ADT' in it. We don't do that anywhere else. =]

================
Comment at: include/llvm/ADT/ADTModCountTracker.h:35-46
@@ +34,14 @@
+
+template <> struct ADTModCountTracker<true> {
+  struct Parent {
+    uint64_t ModCount;
+
+    Parent() : ModCount(0) {}
+
+    void bumpModCount() { ModCount++; }
+
+    ~Parent() { bumpModCount(); }
+  };
+
+  struct Child {
+    const uint64_t *ParentModCountAddress;
----------------
I have to admit, I don't find the naming convention really useful here. Here is my suggested naming. Here are the key points:

1) I would just use the term epoch. That's what you have.
2) I would make it say debugging-only on the tin.
3) I would hint heavily that it is intended to be used as base classes so that the empty base class optimization kicks in.
4) I would handle the NDEBUG stuff at this layer. See more on why in my comment below.


  template <> struct DebugEpochBaseImpl<true> {
    uint64_t Epoch;
  
    struct HandleBase {
      const uint64_t *Epoch;
      uint64_t InitialEpoch;
  
      ...
  
      bool hasEpochAdvanced() const { ... }
    };
  };
  
  #ifndef NDEBUG
  typedef DebugEpochBaseImpl<true> DebugEpochBase;
  #else
  typedef DebugEpochBaseImpl<false> DebugEpochBase;
  #endif

I'm not 100% on "hasEpochAdvanced" though. Maybe there is a better name there. But I would just name these as general Epoch utilities, and I would merge the parent into the containing template. I think handle or ref is a better model than "child".


================
Comment at: include/llvm/ADT/DenseMap.h:52-56
@@ -50,1 +51,7 @@
 
+#ifndef NDEBUG
+typedef ADTModCountTracker<true> DenseMapModCountTracker;
+#else
+typedef ADTModCountTracker<false> DenseMapModCountTracker;
+#endif
+
----------------
Hmm...

What do you think about making this be automatically handled by the epoch utility? That is, localize the NDEBUG behavior there. There is no getting around the fact that this makes data-structures ABI-incompatible between NDEBUG and !NDEBUG, so I'd probably just make it a conditional typedef of the implementation class template.

================
Comment at: include/llvm/ADT/DenseMap.h:1013
@@ -992,2 +1012,3 @@
   friend class DenseMapIterator<KeyT, ValueT, KeyInfoT, Bucket, true>;
+  DenseMapModCountTracker::Child ModCountChild;
 
----------------
This still makes the iterator object larger even when the class is empty.

You need to arrange for this to be the base class of the iteartor so that the empty base optimization kicks in (or to be a base class of a "compressed pair" style member type, but that seems much more complicated than we need).

http://reviews.llvm.org/D7931

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list