Request for review: 8010506 Typos and errors in gc-related flags in globals.hpp
Tao Mao
tao.mao at oracle.com
Thu Oct 3 21:27:38 UTC 2013
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