Request for review: 8010506 Typos and errors in gc-related flags in globals.hpp
Tao Mao
tao.mao at oracle.com
Sat Oct 5 00:08:19 UTC 2013
Hi all,
A new webrev responding to Jon's recent review
http://cr.openjdk.java.net/~tamao/8010506/webrev.05/
Thanks.
Tao
On 10/4/13 4:51 PM, Tao Mao wrote:
> 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