Request for review: 8010506 Typos and errors in gc-related flags in globals.hpp
Tao Mao
tao.mao at oracle.com
Fri Oct 4 23:51:56 UTC 2013
Forwarded Jon's review comments to the main thread.
Tao
On 10/4/13 2:53 PM, Jon Masamitsu wrote:
> If my comments conflict with any other comments, ignore
> might.
>
> product(bool, MaxFDLimit,
> true, \
> - "Bump the number of file descriptors to max in
> solaris.") \
> + "Bump the number of file descriptors to max in
> solaris") \
>
> I would prefer "maximum" instead of "max". There are examples of both
> in the
> file. If we're standarizing on spelling, I prefer t he complete word.
>
>
> 492 product(bool, JavaMonitorsInStackTrace,
> true, \
> 493 "Print info about Java monitor locks when the stacks
> are dumped") \
>
> Spell out "info". Should be "information".
>
>
> product(bool, UseSerialGC,
> false, \
> - "Use the serial garbage
> collector") \
> + "Use the Serial garbage collector")
>
> I think "serial" is more correct that "Serial".
>
>
>
>
> I would have changed this
>
> 584 "Shows oopmap generation")
>
> to
>
>
> 584 "Shows oop map generation")
>
> I don't know what the phase "Show OopMapGeneration" means.
>
> + "Show message about safepoint synch")
> Change "synch" to "synchronization" \
>
>
> + "directory) of the dump file (defaults to
> java_pid<pid>.hprof " \
>
> Why the extra space at the end of " ... java_pid<pid>.hprof "
>
>
>
>
>
> + "Maximum compaction in the Parallel Old garbage collector
> for " \
> "a system GC")
>
>
> "Maximum compaction" -> "Use maximum compaction"
>
>
>
>
> + "Always tenure objects in eden. (Parallel GC
> only)") \
>
> "Parallel GC" does not start a sentence so I would use
> "parallel GC".
>
>
>
> + "Use adaptive free Lists in the CMS
> generation") \
>
> "Lists" -> "lists"
>
> - "Verbose output for parallel
> GC.") \
> + "Verbose output for parallel
> gc") \
>
> I've seen "parallel GC" before in the changes. I
> prefer "parallel GC" to "parallel gc"
>
>
> + "it is disabled because this currently conflicts with
> " \
> + "parallel card scanning under certain
> conditions.") \
>
> Did you intend the period after "conditions"?
>
>
> + "Minimum percentage (0-100) of the CMS incremental duty
> cycle " \
> + "when CMSIncrementalPacing is enable")
>
> prefer "used whenCMSIncrementalPacing is enable" \
>
>
>
>
> + "Use Mark-Sweep-Compact algorithm at full collections")
>
> Prefer "mark-sweek-compact" to "Mark-Sweep-Compact"
>
>
>
>
> + "Whether we should simulate frequent marking stack / work
> queue " \
> + "overflow")
>
>
> "Whether we should simulate" -> "Simulate"
> More consistent with other changes.
>
>
>
> notproduct(uintx, CMSMarkStackOverflowInterval,
> 1000, \
> - "An `interval' counter that determines how
> frequently" \
> - " we simulate overflow; a smaller number increases
> frequency") \
> + "An \"interval\" counter that determines how frequently
> " \
> + "we simulate overflow; a smaller number increases frequency")
>
> "we simulate overflow" -> "to simulate overflow"
>
>
> product(bool, CMSConcurrentMTEnabled,
> true, \
> - "Whether multi-threaded concurrent work enabled (if
> ParNewGC)") \
> + "Whether multi-threaded concurrent work enabled
> " \
> + "(effective only if ParNewGC)")
>
> "Whether multi-threaded concurrent work enabled"
> ->
> "Enablemulti-threaded concurrent work "
>
>
>
> product(uintx, CMSPrecleanThreshold,
> 1000, \
> - "Don't re-iterate if #dirty cards less than
> this") \
> + "Do not re-iterate if number of dirty cards is less than
> this") \
>
>
> "Do not re-iterate ..." -> "Do not iterate again ..."
>
>
> product(bool, UseAdaptiveSizePolicyWithSystemGC,
> false, \
> - "Use statistics from System.GC for adaptive size
> policy") \
> + "Use statistics from System.gc() for adaptive size policy")
>
> "Use statistics" -> "Include statistics"
>
>
> diagnostic(bool, VerifySilently,
> false, \
> - "Don't print print the verification
> progress") \
> + "Do not print print the verification
> progress") \
>
> "print print" -> "print"
>
>
> product(bool, DisableExplicitGC,
> false, \
> - "Tells whether calling System.gc() does a full
> GC") \
> + "Tell whether calling System.gc() does a full
> GC") \
>
> "Tell whether calling" -> "Disable (ignore) calls to"
>
>
>
> - "Print the time taken by each parallel old gc
> phase." \
> - "PrintGCDetails must also be
> enabled.") \
> + "Print the time taken by each ParallelOldGC phase
> " \
> + "(PrintGCDetails must also be enabled)")
>
> I prefer the original with "gc" -> "GC"
>
> or
>
> "Print the time taken by each ParallelOldGC phase"
> ->
> "Print the time taken by each phase with UseParallelOldGC"
>
>
> develop(bool, TraceParallelOldGCMarkingPhase,
> false, \
> - "Trace parallel old gc marking
> phase") \
> + "Trace ParallelOldGC marking phase")
>
> Prefer original with "gc" -> "GC"
>
> or
>
> "Trace parallel old gc marking phase"
>
> ->
> "Trace marking phases with UseParallelOldGC"
>
>
> Similarly with
>
> develop(bool, TraceParallelOldGCSummaryPhase,
> false, \
> - "Trace parallel old gc summary
> phase") \
> + "Trace ParallelOldGC summary
> phase") \
>
> \
> develop(bool, TraceParallelOldGCCompactionPhase,
> false, \
> - "Trace parallel old gc compaction
> phase") \
> + "Trace ParallelOldGC compaction
> phase") \
>
> \
> develop(bool, TraceParallelOldGCDensePrefix,
> false, \
> - "Trace parallel old gc dense prefix
> computation") \
> + "Trace ParallelOldGC dense prefix
> computation") \
>
>
>
>
> + "Rotatingly use gclog files (for long running
> applications). " \
> + "It requires -Xloggc:<filename>")
>
>
> "Rotatingly use gclog" -> "Rotate gclog"
>
>
>
> product(bool, UseCompiler,
> true, \
> - "use
> compilation") \
> + "Use compilation")
>
> "Use compilation" -> "Use Just-In-Time compilation"
>
>
>
> - "how many entries we'll try to leave on the stack during
> " \
> - "parallel
> GC") \
> + "Number of entries we will try to leave on the stack
> " \
> + "during parallel gc"
>
> I prefer "GC" to "gc".
>
> I will probably not have time to respond to any reply
> you have until next week.
>
>
> Jon
On 10/3/13 7:58 PM, Jesper Wilhelmsson wrote:
> Looks good!
> Thanks for doing this!
> /Jesper
>
> Tao Mao skrev 3/10/13 11:27 PM:
>> Hi all,
>>
>> Mikael V. and Jesper's points are taken. There's turned out to be a
>> bunch of
>> changes.
>>
>> Here's the new webrev:
>> http://cr.openjdk.java.net/~tamao/8010506/webrev.04/
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8010506
>>
>> Please review it so that I can get it in before ZBB.
>>
>> Thanks.
>> Tao
>>
>> On 10/2/13 4:54 PM, Jesper Wilhelmsson wrote:
>>> Hi Tao,
>>>
>>> I noticed that in a few places there is a space between the
>>> description string
>>> and the right parenthesis. E.g.
>>>
>>> product(intx, SyncVerbose, 0, "(Unstable)" )
>>>
>>> Search for '" )' and you will find a few.
>>>
>>> There is also a capital S in "LAB'S" in the description of
>>> CMSParPromoteBlocksToClaim.
>>>
>>>
>>> I didn't mention these before since I really want this to be pushed
>>> asap, but
>>> if you decide to act on Mikael's comments about Print vs Prints
>>> maybe you can
>>> fix these minor things as well?
>>>
>>> I don't think a new webrev is needed for these changes.
>>>
>>> Thanks,
>>> /Jesper
>>>
>>>
>>> Mikael Vidstedt skrev 3/10/13 1:40 AM:
>>>>
>>>> Tao,
>>>>
>>>> I sampled the changes and this is definitely a great clean up!
>>>>
>>>> Question: Do you want to attack inconsistencies like "Print" vs
>>>> "Prints" etc.
>>>> since you're changing so much of this anyway?
>>>>
>>>> For example I see that you changed this:
>>>>
>>>> product(bool, ProfileIntervals,
>>>> false, \
>>>> - "Prints profiles for each interval (see
>>>> ProfileIntervalsTicks)") \
>>>> + "Print profiles for each interval (see
>>>> ProfileIntervalsTicks)") \
>>>>
>>>>
>>>> But there's still this:
>>>>
>>>> develop(bool, TraceClearedExceptions,
>>>> false, \
>>>> "Prints when an exception is forcibly
>>>> cleared") \
>>>>
>>>>
>>>> and this:
>>>>
>>>> 3237 develop(intx, TraceBytecodesAt,
>>>> 0, \
>>>> 3238 "Traces bytecodes starting with specified bytecode
>>>> number") \
>>>>
>>>> 3596 develop(bool, PerfTraceDataCreation,
>>>> false, \
>>>> 3597 "Trace creation of Performance Data
>>>> Entries") \
>>>>
>>>> Cheers,
>>>> Mikael
>>>>
>>>> On 2013-10-01 18:00, Tao Mao wrote:
>>>>> Hi all,
>>>>>
>>>>> Jesper's suggestion is integrated.
>>>>>
>>>>> new webrev: "Frame" seems to be the best way to review it.
>>>>>
>>>>> http://cr.openjdk.java.net/~tamao/8010506/webrev.03/
>>>>>
>>>>> Please review it so that I can get it in before ZBB.
>>>>>
>>>>> Thanks.
>>>>> Tao
>>>>>
>>>>> On 9/24/13 12:47 PM, Jesper Wilhelmsson wrote:
>>>>>> Hi Tao,
>>>>>>
>>>>>> The change looks good. Thanks for fixing this!
>>>>>>
>>>>>> Purely esthetic's:
>>>>>> Lines 1820, 1821, 2221 and 2308 in your new globals.hpp needs
>>>>>> their \ aligned.
>>>>>>
>>>>>> Since you are touching pretty much the whole file there are an
>>>>>> inconsistency
>>>>>> that would be nice to get rid of once and for all. The following
>>>>>> flags have
>>>>>> their descriptions end with a period although they are only one
>>>>>> sentence.
>>>>>> Most descriptions do not have the period at the end.
>>>>>>
>>>>>> TracePageSizes
>>>>>> InlineMathNatives
>>>>>> ShowSafepointMsgs
>>>>>> MaxFDLimit
>>>>>> PredictedLoadedClassCount
>>>>>> ParallelOldDeadWoodLimiterMean
>>>>>> ParallelOldDeadWoodLimiterStdDev
>>>>>> ScavengeWithObjectsInToSpace
>>>>>> UseParNewGC
>>>>>> ParallelGCVerbose
>>>>>> ParallelGCBufferWastePct
>>>>>> ParallelGCRetainPLAB
>>>>>> PLABWeight
>>>>>> CMSParPromoteBlocksToClaim
>>>>>> OldPLABWeight
>>>>>> AlwaysPreTouch
>>>>>> CMSExpAvgFactor
>>>>>> CMS_FLSWeight
>>>>>> CMS_FLSPadding
>>>>>> CMS_SweepPadding
>>>>>> CMSDumpAtPromotionFailure
>>>>>> CMSPrintChunksInDump
>>>>>> CMSPrintObjectsInDump
>>>>>> CMSVerifyReturnedBytes
>>>>>> ExecuteInternalVMTests
>>>>>> ConcGCYieldTimeout
>>>>>> PrintParallelOldGCPhaseTimes
>>>>>> ObjectCountCutOffPercent
>>>>>> AbortVMOnExceptionMessage
>>>>>> MaxBCEAEstimateLevel
>>>>>> MaxBCEAEstimateSize
>>>>>> CodeCacheMinimumFreeSpace
>>>>>> CodeCacheMinBlockLength
>>>>>> ExitOnFullCodeCache
>>>>>> Tier0InvokeNotifyFreqLog
>>>>>> Tier2InvokeNotifyFreqLog
>>>>>> Tier3InvokeNotifyFreqLog
>>>>>> Tier0BackedgeNotifyFreqLog
>>>>>> Tier2BackedgeNotifyFreqLog
>>>>>> Tier3BackedgeNotifyFreqLog
>>>>>> Tier3CompileThreshold
>>>>>> Tier4CompileThreshold
>>>>>> IncreaseFirstTierCompileThresholdAt
>>>>>> UsePerfData
>>>>>> PerfDataMemorySize
>>>>>> DumpSharedSpaces
>>>>>>
>>>>>>
>>>>>> Thanks!
>>>>>> /Jesper
>>>>>>
>>>>>>
>>>>>> Tao Mao skrev 27/6/13 7:05 PM:
>>>>>>> new webrev: "Frame" seems to be the best way to review it.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~tamao/8010506/webrev.02/
>>>>>>>
>>>>>>> Suggestion are taken mostly from Thomas.
>>>>>>>
>>>>>>> Thanks.
>>>>>>> Tao
>>>>>>>
>>>>>>> On 5/14/13 2:46 PM, Jon Masamitsu wrote:
>>>>>>>>
>>>>>>>> On 4/30/13 4:58 PM, Tao Mao wrote:
>>>>>>>>> new webrev
>>>>>>>>> http://cr.openjdk.java.net/~tamao/8010506/webrev.01/
>>>>>>>>>
>>>>>>>>> It needs reviews.
>>>>>>>>
>>>>>>>> Tao,
>>>>>>>>
>>>>>>>> Changes you made look good. I did not see a list of the
>>>>>>>> suggested changes in the CR
>>>>>>>>
>>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8010506
>>>>>>>>
>>>>>>>> so don't have any comment about what you decided not
>>>>>>>> to change.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> (2) Identification:
>>>>>>>>> The general rule is to follow the example (product()'s
>>>>>>>>> identification). The
>>>>>>>>> whole file checked and space adjusted accordingly.
>>>>>>>>>
>>>>>>>>> (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?
>>>>>>>>
>>>>>>>> I don't know for sure either but might be because of the Java code
>>>>>>>>
>>>>>>>> public int*hashCode*()
>>>>>>>>
>>>>>>>>
>>>>>>>> Jon
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>> Tao
>>>>>>>>>
>>>>>>>>> On 3/21/13 3:09 PM, Tao Mao wrote:
>>>>>>>>>> 8010506 Typos and errors in gc-related flags in globals.hpp
>>>>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-8010506
>>>>>>>>>>
>>>>>>>>>> webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~tamao/8010506/webrev.00/
>>>>>>>>>>
>>>>>>>>>> Changeset:
>>>>>>>>>> NewRatio should defined as
>>>>>>>>>> product(uintx, NewRatio,
>>>>>>>>>> 2, \
>>>>>>>>>> "Ratio of old/new generation
>>>>>>>>>> sizes") \
>>>>>>>>>> instead of
>>>>>>>>>> product(uintx, NewRatio,
>>>>>>>>>> 2, \
>>>>>>>>>> "Ratio of new/old generation
>>>>>>>>>> sizes") \
>>>>>>>>>>
>>>>>>>>>> PLUS other typos found by Jesper.
>>>>>>>>
>>>>
More information about the hotspot-gc-dev
mailing list