Request for review: 8010506 Typos and errors in gc-related flags in globals.hpp
Tao Mao
tao.mao at oracle.com
Tue Oct 8 15:44:41 UTC 2013
Thank you for reviewing once more, Jesper.
It has touched 500+ lines out of 3800, which means it's more likely to
have conflicts very soon.
Need one more official review to get it in!
Thanks.
Tao
On 10/8/13 8:40 AM, Jesper Wilhelmsson wrote:
> 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