[PATCH] D131707: [analyzer][NFC] Cache the result of getLocationType in TypedValueRegion

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Hmm at a glance I disagree with the FIXME, this doesn't look like a valuable optimization. This operation is typically constant-time anyway so it's probably not worth the memory impact and the added complexity.

The easiest way to roughly estimate impact is to take `sqlite3.c` (which is a famous large-ish project that gets compiled as a single huge file to ensure that optimizations have access to the entire program) and measure time and memory with UNIX `time`. Not very representative but very easy to execute. If it shows promising results I'm open to further investigation in this area.

I also recommend consulting a performance profiler before implementing optimizations, to avoid premature optimizations. IIUC the picture hasn't changed much in the recent years:

- From low-level perspective the majority of time is spent accessing and manipulating `ImmutableMap`s and other immutable data structures in the program state;
- From high-level perspective almost half the time is spent in mark-and-sweep garbage collection in `ExprEngine::removeDead()`. Which can't really be cut as running `removeDead()` less often only increases the analysis time as it makes the maps larger and therefore slower to access. And it's also hard to optimize because the procedure is very complicated and fragile and very few people actually understand how it works.

So I think the most valuable optimizations are low-level optimizations to `ImmutableMap`. There were a few suggestions on the mailing list to use something more modern than the AVL trees under the hood but I don't think authors found much success with those.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131707/new/

https://reviews.llvm.org/D131707