Request for review: 8010506 Typos and errors in gc-related flags in globals.hpp
Jon Masamitsu
jon.masamitsu at oracle.com
Tue Oct 8 21:03:46 UTC 2013
product(double, CMSSmallSplitSurplusPercent, 1.10, \
- "CMS: the factor by which to inflate estimated demand of small" \
- " block sizes to prevent splitting to supply demand for smaller" \
- " blocks") \
+ "CMS: the factor by which to inflate estimated demand of small " \
+ "block sizes to prevent splitting to supply demand for smaller" \
+ "blocks")
Second line of next text, space after "smaller".
product(uintx, CMS_SweepWeight, 75, \
- "Percentage (0-100) used to weight the current sample when " \
+ "Percentage (0-100) used to weigh the current sample when "
I think the original is correct.
weight
/transitive verb/
: to put a weight on (something) to make it heavier or to keep it from
moving
4
*:* to assign a statistical weight to
What do you think?
manageable(intx, CMSAbortablePrecleanWaitMillis, 100, \
- "(Temporary, subject to experimentation)" \
- " Time that we sleep between iterations when not given" \
- " enough work per iteration") \
+ "(Temporary, subject to experimentation) " \
+ "Time that we sleep between iterations when not given" \
+ "enough work per iteration")
Second line of new text, space after "given"
notproduct(bool, PrintFlagsWithComments, false, \
- "Print all VM flags with default values and descriptions and exit")\
+ "Print all VM flags with default values and descriptions and exit") \
Not sure on this. Sometimes I don't see the right number of
blanks but do you want a separate line so "\" line up?
Thanks all.
Jon
On 10/4/2013 5:08 PM, Tao Mao wrote:
> 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.
>>>>>>>>>>
>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20131008/1859f819/attachment.htm>
More information about the hotspot-gc-dev
mailing list