Request for review: 8010506 Typos and errors in gc-related flags in globals.hpp
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Fri Oct 4 02:58:10 UTC 2013
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