[PATCH] D43430: Omit nullptr check for sufficiently simple delete-expressions

[Andrew Hunter via Phabricator via cfe-commits]

ahh added a comment.

> If the pointer is not null, the runtime overhead of the null check is pretty negligible next to the cost of actually doing the allocation. If the pointer is null, the runtime overhead of making at least one unnecessary call — probably two, if 'operator delete' doesn't do its own null check before calling 'free', and probably one that crosses image boundaries — is not negligible at all. So the relative impact on code that does end up destroying a trivial value is outsized.

In a reply of mine that I think got eaten by list moderation,  I looked into this and benchmarked the cost of ::operator delete; with our tcmalloc, the cost of deleting null is about 8 cycles (compared to an empty loop.) (I don't really know how to benchmark the version with an if around it, but if we assume that's free, 8 cycles is still very cheap.)

I suppose this might go up somewhat in an environment where we have to make some sort of PLT call or even two. My Google centric response is "don't do that"--linking directly against any modern malloc should avoid any jump in `::operator delete` and our environment make direct calls quite fast; I'm not sure how generalizable that is. (The linking is I think universally good advice; I'm not sure who runs in an environment that cannot make efficient far calls. But point is that: while your statement is true, the penalty for getting this wrong seems very small, and as you say any programmer can if around it at a "hot" null delete, no?

This is one of the few aspects of malloc calls that I don't have near-infinite telemetry for (our sampling architecture doesn't easily collect it.)  So I cannot give you a hard number of the fraction of deletes that are of null pointers, but I am convinced that is very small.  Would more (Google-internal, obviously) data on this make a decision easier?

I could see why maybe this could be gated on -Os, but I didn't do this for two reasons:

- I am new at Clang development and wasn't sure how to put that sort of a check in :) Though I can learn if this is a hard requirement.
- From our perspective, I think Google would want this flag in non-size optimization (-O2 or whatever.)  We delete null infrequently enough that I'd expect this to be a pure cycle win (if a very small one) and even though (because?) we don't optimize for size, we have a number of very large binaries, and reducing icache hit can help a lot.

I'm unsure exactly how to make progress here, since for one thing I'm unsure how strongly you feel about the potential cost/benefits. Guidance would be greatly appreciated!


Repository:
  rC Clang

https://reviews.llvm.org/D43430