[PATCH] D71104: scudo: Add a basic malloc/free benchmark.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked 3 inline comments as done.
pcc added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/benchmarks/malloc_benchmark.cpp:33
+    auto *Data = reinterpret_cast<uint8_t *>(Ptr);
+    for (size_t I = 0; I < NBytes; I += PageSize)
+      Data[I] = 1;
----------------
hctim wrote:
> hctim wrote:
> > Can we instead do what bionic-benchmarks does on their malloc test, and touch each page to ensure residency (although we're only allocating 8-bytes at a time).
> > 
> > Similarly, they also bulk allocate -> bulk deallocate using a storage vector. Seems like a better solution - so we don't just hit the freelist `128 * 1024` times.
> I have absolutely no idea why I read this loop as memcpy, but please ignore the first point.
> 
> Bulk allocate/delete would still be great.
> Similarly, they also bulk allocate -> bulk deallocate using a storage vector.

Are we looking at different benchmarks? This is what I've been using: 
https://android.googlesource.com/platform/bionic/+/master/benchmarks/stdlib_benchmark.cpp#41

I guess a storage vector would be more realistic, although it may end up conflating the speed of the allocator's main path with other things (e.g. mmap performance, reclaiming, cache (but maybe not because of the quarantine?)) so maybe it should be a separate benchmark?

> we don't just hit the freelist 128 * 1024 times.

(that wouldn't happen, the number here is the number of *bytes* to allocate, not the number of iterations)


================
Comment at: compiler-rt/lib/scudo/standalone/benchmarks/malloc_benchmark.cpp:42
+
+BENCHMARK_TEMPLATE(BM_malloc_free, scudo::AndroidConfig)->Range(8, 128 * 1024);
+BENCHMARK_TEMPLATE(BM_malloc_free, scudo::AndroidSvelteConfig)
----------------
hctim wrote:
> Missing `DefaultConfig`?
Testing with `DefaultConfig` caused crashes for me, I think it was a D70760 style problem with the exclusive TSD but I couldn't seem to solve it. I'll add a FIXME here.


================
Comment at: compiler-rt/lib/scudo/standalone/benchmarks/malloc_benchmark.cpp:46
+#if SCUDO_CAN_USE_PRIMARY64
+BENCHMARK_TEMPLATE(BM_malloc_free, scudo::FuchsiaConfig)->Range(8, 128 * 1024);
+#endif
----------------
hctim wrote:
> Can we make `128 * 1024` and `8` constants?
Sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71104