vsavchenko added inline comments.
================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:88 + // structure is preferred. + using ImplType = llvm::SmallVector<Range, 4>; + ---------------- vsavchenko wrote: > NoQ wrote: > > vsavchenko wrote: > > > grandinj wrote: > > > > Just curious - if they mostly contain 1 or 2 elements, why is N == 4 > > > > here? > > > Essentially, the main factor is **intuition** 😆 > > > The main idea here is that allocations are the single most expensive > > > thing about ranges, so I want to avoid extra allocations not for 60% of > > > range sets, but for 90%. I still should definitely measure performance > > > difference for different values of N and gather some stats about the > > > lengths. > > Given that you pass those by pointers, i suspect you don't need to fix the > > size at all. In fact the small-size of the small vector is, by design, a > > //runtime// value and you can use `llvm::SmallVectorImpl *` instead, which > > can point to `SmallVector` of any small-size. This would allow you to > > allocate small vectors of variable length which you can take advantage of > > whenever you know the length in advance. > Sounds reasonable, I'll give it a shot! On the second thought, it's not going to be pretty. I still will need to allocate `SmallVector<..., N>`, where `N` is a compile-time constant, so it would be a big old switch where I "convert" run-time values into compile-time values so-to-speak. Additionally, the involvement of `SmallVector` doesn't seem to be necessary at all because in this setting we don't use the main feature of it that it can grow larger than this size onto the heap. So, we can simply do the same trick with `ArrayRef` and `std::array`, but with less ambiguity. However, one of the benefits of the current approach is "let's allocate as little as possible". It does the operation, checks if we allocated space for a container with these contents, and allocates it only if not. It is MUCH cheaper to do the operation and find out that we already have one like this or copy it to the newly allocated place than allocating it every time. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86465/new/ https://reviews.llvm.org/D86465 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
