[PATCH] D73507: [scudo][standalone] Secondary & general other improvements

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked 4 inline comments as done.
cryptoad added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/flags.inc:48
 
-SCUDO_FLAG(int, release_to_os_interval_ms, 5000,
+SCUDO_FLAG(int, release_to_os_interval_ms, SCUDO_ANDROID ? 1000 : 5000,
            "Interval (in milliseconds) at which to attempt release of unused "
----------------
pcc wrote:
> hctim wrote:
> > Given that this might get very complicated if you wanted to introduce different defaults for different platforms - it may be worth adding a TODO to initialise Scudo as part of `libc_init_malloc` rather than rely on lazy-initialisation.
> I would prefer if we moved towards making these things static (e.g. part of AndroidConfig et al) rather than configurable via environment variables.
Ack. Technically a platform could also use the SCUDO_DEFAULT_OPTIONS define at compile time.


================
Comment at: compiler-rt/lib/scudo/standalone/primary64.h:88
       // limit is mostly arbitrary and based on empirical observations.
       // TODO(kostyak): make the lower limit a runtime option
       Region->CanRelease = (ReleaseToOsInterval >= 0) &&
----------------
hctim wrote:
> Might be worth considering
In the works with https://reviews.llvm.org/D72882. I'll come back to that after this lands as it's higher priority.


================
Comment at: compiler-rt/lib/scudo/standalone/secondary.h:146
+private:
+  void empty() {
+    struct {
----------------
hctim wrote:
> Can you reuse code via. `empty() { releaseOlderThan(UINT64_MAX) }`?
I looked into this, as the constructs are similar. One of the issues is that one works with `Map*` members, the other with `Block*` members.
In the end I decided against as it became ugly, but I can revisit this at a later point hopefully.


================
Comment at: compiler-rt/lib/scudo/standalone/secondary.h:193
+        N++;
+      }
+    }
----------------
hctim wrote:
> Missing `EntriesCount -= N`?
The entries are still usable, we released the memory but they can be reused at some point (unlike unmapping). It will incur faulting, which is slower, but has a positive impact on the RSS.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73507