Request for review: 8010506 Typos and errors in gc-related flags in globals.hpp

[Thomas Schatzl]

Hi,

On Tue, 2013-04-30 at 16:58 -0700, Tao Mao wrote:
> new webrev
> http://cr.openjdk.java.net/~tamao/8010506/webrev.01/
> 
> It needs reviews.
> 
> Changeset:
> Thank you for the thorough report of all these typos/errros, Thomas.
> 
> I changed most places according to your suggestions (for the rest few, I 
> did not change because it may slightly change the meaning of the 
> definition). Also, I went through the file to change some similar common 
> errors that occurred.
> 
> Several issues:
> 
> (1) parallel gc should be lower case or upper case?
> As I know and, perhaps, someone told me, the term "parallel gc" 
> sometimes refers to a more generic terminology rather than ParallelGC or 
> ParallelOldGC, if it doesn't imply these two. Basically, any GC 
> algorithms that can be parallelized could fall into this category. Right 
> now, we probably refer "parallel gc" to ParallelGC or ParallelOldGC. 
> But, we reserve the right to change it to something else. That's why 
> "fuzziness" exists here. I guess the same way to deal with "concurrent 
> gc". So, I didn't make these words upper cased.

Okay. Looking into the code where it is used would help disambiguating
the meaning. Still I would upper case the "GC" as an abbreviation in
these cases - but that's style.

> 
> (2) Identification:
> The general rule is to follow the example (product()'s identification). 
> The whole file checked and space adjusted accordingly.

Thanks :)

> 
> (3) Line breaks
> A space should be provided at the line breaks if multiple lines is 
> needed. Some lines that are slightly longer than normal length is kept 
> untouched as long as they read well.
>
> *(4) Why does the VM option hashCode have lower case initial?
> The direct answer is "I don't know". (But it is understandable that 
> hashCode is "legacy" such that it has it own style) Anyone knows the answer?
> 

A few more comments on the patch:

LargePageSizeInBytes: missing closing bracket in the text
ForceUnreachable: Make all non code cache addresses to be unreachable
*by* ...
StressDerivedPointers: ... when a derived pointer...
ZombieALot: "runt." -> "runtime"
LogEventsBufferEntries: has the same text as LogEvents. It should
probably be "Number of event log buffers" or so.
ZapStackSegments: "Stack" -> "stack"
HeapDumpBeforeFullGC/HeapDumpAfterFullGC: I think the term is
"stop-the-world GC"
AutoShutdownNMT: "situation" -> "situations"
ProfilerRecordPC: "tick" -> "ticks" (I think, see the next few)
UnsyncloadClass: there are two subsequent spaces in the text.
DontYieldALot, ConvertSleepToYield, ConvertYieldToSleep, and a few more:
SOLARIS -> Solaris
SyncFlags: missing space between Unsafe, Unstable
AdjustConcurrency: "call" -> "Call", at thread *creation* time
StressMethodComparator: "run" -> "Run"
TraceNewOopMapGeneration, TraceNewOopMapGenerationDetailed, TraceMonitorMismatch: "OopMapGeneration" -> "oop map generation" (it's done later that way)
ParallelOldDeadWoodLimiterMean, ParallelOldDeadWoodLimiterStdDev: "par compact" -> "parallel compaction"; maybe remove "a number" in the brackets
ParallelGCThreads,GCTaskTimeStampEntries: "gc" -> "GCs" (it's used as an abbreviation here I guess)
UseDynamicNumberOfGCThreads, ForceDynamicNumberOfGCThreads: "parallel gc" -> "the parallel GCs"
ConcGCThreads: I think this description is outdated, as G1 also uses this variable: something like "Number of concurrent threads used" maybe?
YoungPLABSize, OldPLABSize, ResizePLAB, PrintPLAB, CMSParPromoteBlocksToClaim, ResizeOldPLAB, PrintOldPLAB: I think the tick "'" is wrong here, as plural is meant. E.g LAB's -> LABs
NeverTenure: "May" -> "may", "ParallelGC" -> "Parallel GC" (as above)
ExplicitGCInvokesConcurrent, ExplicitGCInvokesConcurrentAndUnloadsClasses: "effective only when UseConcMarkSweepGC" -> "Concurrent Mark-Sweep GC only" (similar to "Parallel GC only" above)
CMSParallelSurvivorRemarkEnabled, CMSPLABRecordAlways, CMSConcurrentMTEnabled: sometimes it is "(effective only if", sometimes "(effective only when", sometimes just "(if ..."
GCLockerInvokesConcurrent: "CS" -> "critical section"
ParallelGCVerbose: "parallel GC" -> "parallel GCs" (it's used in ParNew and G1)
TargetPLABWastePct: pct -> percent
ParGCArrayScanChunk: object array -> object arrays
AlwaysPreTouch: "It forces..." -> "Force all ..."

CMSIncrementalMode: quotation signs: (not only here, in general) sometimes "" is used to quote, sometimes `'
FLSLargestBlockCoalesceProximity: I think it should be "coalescing force", not "coalition force". But ask around first :)
>From FLSAlwaysCoalesceLarge to CMSExtrapolateSweep we use the prefix "CMS:" to indicate it is CMS only. Earlier we added a bracket reading (XY only) at the end.
SweepWeight: weight -> weigh
CMSPrintChunksInDump, CMSPrintObjectsInDump, FLSVerifyAllHeapReferences, PrintPromotionFailure, DeferInitialCardMark:: space at beginning of second line of text
VerifyBlockOffsetArray: maybe remove the exclamation mark, the other options also don't use it
RefDiscoveryPolicy: -> "Select type of reference discovery policy, reference-based(0) or referent-based(1)"
ObjArrayMarkingStride: "ObjArray" -> "object array"
MetadataAllocationFailALotInterval, CMSCoordinatorYieldSleepCount, CMSYieldSleepCount: lower case initial
PrintTLAB, TLABStats: same description; I think TLABStats should read: "Provide more detailed and expensive TLAB statistics (with PrintTLAB)"
UseAdaptiveSizePolicyWithSystemGC: System.GC -> System.gc()
MaxGCPauseMillis: msec -> ms, max. -> maximum (not sure if msec is an official unit in the US, but the SI unit is ms)
MaxGCMinorPauseMillis: msec -> ms
BaseFootPrintEstimate: "Estimate of footprint for space other than the Java heap"
GCTimeLimit: of proportion->the proportion
VerifySilently: Don't -> Do not
PrintGC, PrintGCDetails, PrintGCDateStamps, PrintGCTimeStamps: "collect" -> "collection"
PrintGCTaskTimeStamps, TraceParallelOldGCMarkingPhase, TraceParallelOldGCSummaryPhase: gc -> GC
ConcGCYieldTimeout: # -> amount
PrintParallelOldGCPhaseTimes: "PrintGCDetails must also be enabled" - previously just "(with XXX)" has been used.
TraceParallelOldGCCompactionPhase, TraceParallelOldGCDensePrefix: "parallel old"
UseGCLogFileRotation: "Requires -Xloggc:<filename>", previously "with XXX" (I like "requires XXX" best)
NumberOfGCLogFiles: casing after comma. I think the comma should be a full stop. We previously also didn't say "Default: <whatever>", but just "0 means <whatever>"
GCLogFileSize: "Only valid with ..."
(I skipped the "compiler interface" and everything until "gc parameters" as it is full of initial lower casing and not gc related; I don't understand why sometimes these changes fixes that, and sometimes not. The CR is about "gc-related" flags...)
We should fix the default value of MaxNewSize meaning use ergonomics to zero just like the others.
MarkSweepDeadRatio: "Serial mark sweep" -> "Serial GC"; "Par compact" -> "Parallel GCs"?
PSChunkLargeArrays: "true; process" -> "Process" (skip the "true", it seems to be the default value); it seems that every GC has its own object array chunking parameter :(
GCDrainStackTargetSize: initial lower case
PerfDataSaveFile: maybe "... absolute pathname, the string %p will be replaced by the PID"

- Maybe: Change "(For XXX only)" to "(XXX only)" or fix capitalization. Also sometimes it is "(For", sometimes "(for"
- The change adds a full stop at the end of some strings.
- Especially at the beginning the verbage/style of the text is different, i.e. [It] "Print*s* <whatever>" vs. "Print <whatever>". I think the later style is used much more often.

Thanks for looking at that,
Thomas