Request for review: 8010506 Typos and errors in gc-related flags in globals.hpp
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Tue Oct 8 15:40:53 UTC 2013
Tao,
Just getting better and better :-)
Ship it!
/Jesper
Tao Mao skrev 5/10/13 2:08 AM:
> 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