Details
- Reviewers
bthrall - Commits
- rFIREFOXAUTOLANDe3887a62b670: Bug 2062535 - Migrate to using printf_stderr in the GC r=bthrall
- Bugzilla Bug ID
- 2062535
Diff Detail
- Repository
- rFIREFOXAUTOLAND firefox-autoland
Event Timeline
(Reviewing Diff 1354796)
Summary of Changes
Intent
The purpose of these changes is to migrate all fprintf(stderr, ...) calls in the SpiderMonkey garbage collector (GC) codebase to use printf_stderr(...) instead. This is a straightforward mechanical refactoring to standardize how diagnostic and debug output is written to stderr across the GC subsystem. The printf_stderr function (from mozilla/glue/Debug.h) provides a platform-aware abstraction for writing to stderr that is preferred within the Mozilla/Firefox codebase over direct fprintf(stderr, ...) calls.
Solution
The implementation is a systematic find-and-replace across seven GC source files (Verifier.cpp, Tenuring.cpp, Statistics.cpp, Scheduling.cpp, Pretenuring.cpp, Nursery.cpp, Marking.cpp, Heap.cpp, and GC.cpp):
- Added #include "mozilla/glue/Debug.h" to each affected file to bring in the declaration of printf_stderr.
- Replaced all fprintf(stderr, ...) calls with printf_stderr(...), removing the stderr first argument since printf_stderr implicitly writes to stderr. This covers a wide variety of GC diagnostic output including:
- Incremental marking verification error messages
- Heap check failure reports and cell info dumps
- Black-to-gray edge violation reports
- WeakMap marking consistency error messages
- Memory tracker leak diagnostics
- Pretenuring statistics and alloc site info tables
- Nursery promotion statistics and deduplication data
- Minor/major GC scheduling debug output
- Arena info dumps
- Zeal mode help text
- Zone scheduling debug information
The changes are purely mechanical with no logic modifications — only the output function and its argument list are adjusted, while the format strings and their arguments remain identical. One notable exception is that a single fputs(ZealModeHelpText, stderr) call was left unchanged, as it uses a different output function.
| js/src/gc/GC.cpp | ||
|---|---|---|
| 890 | The fputs(ZealModeHelpText, stderr) on line 893 still writes directly to stderr while all surrounding calls now use printf_stderr. On Android, the printf_stderr output goes to logcat but fputs output goes to stderr (likely invisible). Convert to printf_stderr("%s", ZealModeHelpText) for consistency. | |
| js/src/gc/Pretenuring.cpp | ||
| 484–485 | printf_stderr implicitly completes a logcat line on Android even without a trailing newline. This function builds a single table row across ~9 printf_stderr calls (lines 486–539), which will produce 9 separate logcat entries instead of one formatted line on Android. Build the full line in a buffer (e.g., using SprintfLiteral or a StringBuilder) and output it with a single printf_stderr call. | |
| js/src/gc/Verifier.cpp | ||
| 966–967 | printf_stderr implicitly completes a logcat line per call on Android. dumpCellInfo builds a single line across 3–5 calls (lines 961–971), and its callers dumpCellPath (line 978) and onChild (line 1103) add even more calls around it. The output will be fragmented into separate logcat entries on Android. Build the full line in a buffer and output it with a single call. | |
This looks good enough to land, and makes things better for Android as the bug intends. It is up to you if you want to make the effort to follow reviewhelper-bot's suggestions.
| js/src/gc/Heap.cpp | ||
|---|---|---|
| 189–196 | This would be nice as a single call to printf_stderr(). | |
| js/src/gc/Pretenuring.cpp | ||
| 484–485 | I agree, it would be an improvement to use one printf_stderr() call at the end instead of all these separate calls here. I don't think you need a buffer in this case, though. | |
| js/src/gc/Verifier.cpp | ||
| 966–967 | It would be nice to collect these into fewer printf_stderr() calls, too (one per dumpCellInfo() perhaps?) but you don't have to bend over backwards to do it. | |
| js/src/gc/Pretenuring.cpp | ||
|---|---|---|
| 506 | Since we can't reuse buffer now, it could have a clearer name. | |
| 523–527 | I don't think this comment helps any more. | |
| js/src/gc/Verifier.cpp | ||
| 966–977 | The hardcoded buffer sizes is potentially limiting; you could do this as two printf_stderr calls (using if (ob) { ... } else { ... } ) to avoid that. Given that this is code for debugging, though, I don't think it makes much of a difference. | |