Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This needs a stub GetRSS for Fuchsia, at least.
Since there's no way to get a useful value for this on Fuchsia as yet, it would be preferable to conditionalize the machinery so it isn't doing no-op work.
The feature right now is only supported on LINUX. isRssLimitExceeded() is protected by #if and inlined in any non-linux system and a generic GetRSS symbol is exported in any non-Linux systems (so even with LD_BIND_NOW the symbol exists/resolves). GetRSS() never gets called since it is only called by isRssLimitExceeded() and by a flow conditioned in the isRssLimitExceeded result.
With optimizations on, even the references to the functions and the entire conditional block get removed.
Can you reupload the patch with full context?
compiler-rt/lib/scudo/standalone/linux.cpp | ||
---|---|---|
185 | The opening brace should be on the previous line. | |
190 | The opening brace should be on the previous line. | |
195 | The opening brace should be on the previous line. | |
compiler-rt/lib/scudo/standalone/report.cpp | ||
40 | The opening brace should be on the previous line. |
Fixed the few style errors pointed out (thank you!) and submitting a full context patch.
Fixed the few style errors pointed out (thank you!) and submitting a full context patch.
Fixed compiler warnings due to implicit type conversions, got more pedantic on using the right types (even for compatible ones).
compiler-rt/lib/scudo/standalone/atomic_helpers.h | ||
---|---|---|
127 ↗ | (On Diff #440238) | This should use the call above with true instead of false for the bool argument. |
compiler-rt/lib/scudo/standalone/combined.h | ||
351 | excess space between parens | |
737 | It's better not to use ALWAYS_INLINE and leave it to the compiler to decide (which it will). | |
741 | Use false, not 0. | |
1019 | Put spaces around the =. |
compiler-rt/lib/scudo/standalone/atomic_helpers.h | ||
---|---|---|
127 ↗ | (On Diff #440238) | please move this into a separate patch and extend compiler-rt/lib/scudo/standalone/tests/atomic_test.cpp for context, you can organize patches into "stack" using "edit related revision" in the Phabricator menu. |
compiler-rt/lib/scudo/standalone/combined.h | ||
351 | just clangformat entire patch | |
355 | static_cast | |
compiler-rt/lib/scudo/standalone/flags.inc | ||
49 | limits need own tests as well | |
compiler-rt/lib/scudo/standalone/linux.cpp | ||
197 | How about moving GetRSS() (with a test) implementation into a separate patch? |
Bastian Kersting started splitting the patch and making the suggested changes. The first in the series is here: https://reviews.llvm.org/D138406 - I appreciate if folks here help reviewing/merging.
Can you please rebase this one?
compiler-rt/lib/scudo/standalone/combined.h | ||
---|---|---|
708 | can you measure a meaningful difference or real apps between weak vs strong here? |
compiler-rt/lib/scudo/standalone/combined.h | ||
---|---|---|
708 | I benchmarked this method and found not a big difference but a difference between weak and strong here. I benchmarked this on an ARM (aarch64) machine with the following findings: 2022-12-15T09:02:48-08:00 Running third_party/llvm/llvm-project/compiler-rt/scudo_benchmarks Run on (64 X 3000 MHz CPU s) CPU Caches: L1 Data 64 KiB (x64) L1 Instruction 64 KiB (x64) L2 Unified 1024 KiB (x64) L3 Unified 32768 KiB (x1) Load Average: 4.02, 9.90, 14.71 ----------------------------------------------------------------- Benchmark Time CPU Iterations ----------------------------------------------------------------- BM_IsRssExceeded 47.6 ns 47.5 ns 14772023 BM_IsRssExceededStrong 47.8 ns 47.7 ns 14673854 I get that this is not a big difference, but I would still argue that it is enough to use weak here. We could introduce a compilation parameter with the default weak. |
compiler-rt/lib/scudo/standalone/combined.h | ||
---|---|---|
167 | zero initialized value will work the same | |
699 | Probably nicer to put this method and related fields into another class, e.g class RssLimitChecker { atomic_u64 RssLastCheckedAtNS = {}; atomic_u8 RssLimitExceeded = {}; // Slow part in cpp file bool Check(); public: // Inline fast path. bool isExceeded() { u64 LastCheck = atomic_load_relaxed(&RssLastCheckedAtNS); const u64 CurrentCheck = getMonotonicTime(); if (UNLIKELY(CurrentCheck >= LastCheck + (250ULL * 1000000ULL))) Check(); return atomic_load_relaxed(&RssLimitExceeded); } } | |
705 | instead of adding interval here, CurrentCheck could be named e.g. "Now". | |
708 | Then please just switch to strong | |
723–730 | ||
compiler-rt/lib/scudo/standalone/tests/combined_test.cpp | ||
776 | This relies on the order of the test execution for avoid this the test should backup and restore flags | |
792 | this code does not use {} for one line loop bodies | |
799 | restore flags |
This looks good to me with nits
compiler-rt/lib/scudo/standalone/combined.h | ||
---|---|---|
165 | remove {} | |
351–352 | redundant () | |
compiler-rt/lib/scudo/standalone/common.h | ||
223 | atomic_compare_exchange_strong is in UNLIKELY branch already | |
241 | Remove redundant ULL as result fits into int32 | |
compiler-rt/lib/scudo/standalone/linux.cpp | ||
229 | remove {} | |
230 | maybe nicer to RssLimitExceeded -> enum {Neither, Soft, Hard} |
you don't need getOptionsForTesting if you can run init after updating flags
compiler-rt/lib/scudo/standalone/combined.h | ||
---|---|---|
165 | get from RssChecker | |
356 | please add and use RssChecker.getSoftRssLimit() RssChecker.getHardRssLimit() | |
compiler-rt/lib/scudo/standalone/linux.cpp | ||
216 | Return here if they are both zeroes and eliminate CheckRssLimit? |
I also restructured the tests to get rid of getOptionsForTesting and the flags since this is now read at the creation of the RssChecker.
compiler-rt/lib/scudo/standalone/common.h | ||
---|---|---|
223 | That would mean Check needs the NextCheck variable for context and that would become a parameter of Check right? | |
compiler-rt/lib/scudo/standalone/linux.cpp | ||
216 | Done. What do you mean by "eliminate CheckRssLimit"? |
remove {}