Looks good to me!
/R
On May 9, 2013, at 8:38 AM, 云达(Yunda) wrote:
> Rickard,
>
> Actually I've considered the option and I just thought it's less elegant. But
> just as you said, it's clearer and easier to read. The updated and tested
> change:
>
> diff -r 627cf9e9ea31 src/share/vm/runtime/globals.hpp
> --- a/src/share/vm/runtime/globals.hpp Mon May 06 10:29:49 2013 -0700
> +++ b/src/share/vm/runtime/globals.hpp Thu May 09 14:36:35 2013 +0800
> @@ -3634,7 +3634,10 @@
> "Include GC cause in GC logging") \
> \
> product(bool, EnableTracing, false, \
> - "Enable event-based tracing")
> + "Enable event-based tracing") \
> + \
> + product(bool, UseLockedTracing, false, \
> + "Use locked-tracing when doing event-based tracing")
>
> /*
> * Macros for factoring of globals
> diff -r 627cf9e9ea31 src/share/vm/trace/traceEventClasses.xsl
> --- a/src/share/vm/trace/traceEventClasses.xsl Mon May 06 10:29:49 2013 -0700
> +++ b/src/share/vm/trace/traceEventClasses.xsl Thu May 09 14:36:35 2013 +0800
> @@ -132,6 +132,15 @@
> void writeEvent(void) {
> ResourceMark rm;
> HandleMark hm;
> + if (UseLockedTracing) {
> + ttyLocker lock;
> + writeEventContent();
> + } else {
> + writeEventContent();
> + }
> + }
> +
> + void writeEventContent() {
> TraceStream ts(*tty);
> ts.print("<xsl:value-of select="@label"/>: [");
> <xsl:apply-templates select="value|structvalue" mode="write-data"/>
>
>
> Regards,
> Yunda
>
>> -----Original Message-----
>> From: Rickard Bäckman [mailto:rickard.back...@oracle.com]
>> Sent: Wednesday, May 08, 2013 3:03 PM
>> To: David Holmes
>> Cc: 云达(Yunda); serviceability-dev@openjdk.java.net;
>> hotspot-runtime-...@openjdk.java.net
>> Subject: Re: [PATCH] EnableTracing: output from multiple threads may be mixed
>> together
>>
>> One option could be to move the content of writeEvent() to its own method
>> (maybe writeEventContent()) and just do
>>
>> if (UseLockedTracing) {
>> ttyLocker lock;
>> writeEventContent();
>> } else {
>> writeEventContent();
>> }
>>
>> I think it makes it easier to read.
>>
>> /R
>>
>> On May 8, 2013, at 2:41 AM, David Holmes wrote:
>>
>>> On 7/05/2013 10:33 PM, 云达(Yunda) wrote:
>>>> David,
>>>>
>>>> Sorry about the stupid mistake. And please see the tested change below.
>> Since I don't find an elegant way to define a ttyLocker object, I use the
>> static
>> methods of ttyLocker instead.
>>>
>>> Yeah there's no elegant solution when you need to conditionalize
>>> things. :(
>>>
>>> This looks fine to me but the serviceability folk should chime back in.
>>>
>>> Thanks,
>>> David
>>>
>>>> diff -r 627cf9e9ea31 src/share/vm/runtime/globals.hpp
>>>> --- a/src/share/vm/runtime/globals.hpp Mon May 06 10:29:49 2013
>>>> -0700
>>>> +++ b/src/share/vm/runtime/globals.hpp Tue May 07 20:04:24 2013
>>>> +++ +0800
>>>> @@ -3634,7 +3634,10 @@
>>>> "Include GC cause in GC logging")
>> \
>>>>
>> \
>>>> product(bool, EnableTracing, false,
>> \
>>>> - "Enable event-based tracing")
>>>> + "Enable event-based tracing")
>> \
>>>> +
>> \
>>>> + product(bool, UseLockedTracing, false,
>> \
>>>> + "Use locked-tracing when doing event-based
>>>> + tracing")
>>>>
>>>> /*
>>>> * Macros for factoring of globals
>>>> diff -r 627cf9e9ea31 src/share/vm/trace/traceEventClasses.xsl
>>>> --- a/src/share/vm/trace/traceEventClasses.xsl Mon May 06 10:29:49
>>>> 2013 -0700
>>>> +++ b/src/share/vm/trace/traceEventClasses.xsl Tue May 07 20:04:24
>>>> +++ 2013 +0800
>>>> @@ -132,10 +132,17 @@
>>>> void writeEvent(void) {
>>>> ResourceMark rm;
>>>> HandleMark hm;
>>>> + int holder;
>>>> + if (UseLockedTracing) {
>>>> + holder = ttyLocker::hold_tty();
>>>> + }
>>>> TraceStream ts(*tty);
>>>> ts.print("<xsl:value-of select="@label"/>: [");
>>>> <xsl:apply-templates select="value|structvalue" mode="write-data"/>
>>>> ts.print("]\n");
>>>> + if (UseLockedTracing) {
>>>> + ttyLocker::release_tty(holder);
>>>> + }
>>>> }
>>>> };
>>>>
>>>>
>>>> Regards,
>>>> Yunda
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: David Holmes [mailto:david.hol...@oracle.com]
>>>>> Sent: Tuesday, May 07, 2013 5:50 PM
>>>>> To: 云达(Yunda)
>>>>> Cc: hotspot-runtime-...@openjdk.java.net;
>>>>> serviceability-dev@openjdk.java.net
>>>>> Subject: Re: [PATCH] EnableTracing: output from multiple threads may
>>>>> be mixed together
>>>>>
>>>>> On 7/05/2013 7:43 PM, 云达(Yunda) wrote:
>>>>>> Hi David,
>>>>>>
>>>>>> Thanks for the review and I see your concern. Please see the
>>>>>> updated change
>>>>> below:
>>>>>>
>>>>>> diff -r 627cf9e9ea31 src/share/vm/runtime/globals.hpp
>>>>>> --- a/src/share/vm/runtime/globals.hpp Mon May 06 10:29:49 2013
>>>>>> -0700
>>>>>> +++ b/src/share/vm/runtime/globals.hpp Tue May 07 17:38:58 2013
>>>>>> +++ +0800
>>>>>> @@ -3634,7 +3634,10 @@
>>>>>> "Include GC cause in GC logging")
>>>>> \
>>>>>>
>>>>> \
>>>>>> product(bool, EnableTracing, false,
>>>>> \
>>>>>> - "Enable event-based tracing")
>>>>>> + "Enable event-based tracing")
>>>>> \
>>>>>> +
>>>>> \
>>>>>> + product(bool, UseLockedTracing, false,
>>>>> \
>>>>>> + "Use locked-tracing when doing event-based
>>>>>> + tracing")
>>>>>>
>>>>>> /*
>>>>>> * Macros for factoring of globals diff -r 627cf9e9ea31
>>>>>> src/share/vm/trace/traceEventClasses.xsl
>>>>>> --- a/src/share/vm/trace/traceEventClasses.xsl Mon May 06 10:29:49
>>>>>> 2013 -0700
>>>>>> +++ b/src/share/vm/trace/traceEventClasses.xsl Tue May 07 17:38:58
>>>>>> +++ 2013 +0800
>>>>>> @@ -132,6 +132,9 @@
>>>>>> void writeEvent(void) {
>>>>>> ResourceMark rm;
>>>>>> HandleMark hm;
>>>>>> + if (UseLockedTracing) {
>>>>>> + ttyLocker ttyl;
>>>>>> + }
>>>>>
>>>>> Ah that doesn't work - the ttyLocker is block-scoped by the if
>>>>> statement so it will be created, grab the lock, then immediately be
>>>>> destructed and the lock released.
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>> TraceStream ts(*tty);
>>>>>> ts.print("<xsl:value-of select="@label"/>: [");
>>>>>> <xsl:apply-templates select="value|structvalue"
>>>>>> mode="write-data"/>
>>>>>>
>>>>>> Regards,
>>>>>> Yunda
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: David Holmes [mailto:david.hol...@oracle.com]
>>>>>>> Sent: Tuesday, May 07, 2013 10:39 AM
>>>>>>> To: 云达(Yunda)
>>>>>>> Cc: hotspot-runtime-...@openjdk.java.net;
>>>>>>> serviceability-dev@openjdk.java.net
>>>>>>> Subject: Re: [PATCH] EnableTracing: output from multiple threads
>>>>>>> may be mixed together
>>>>>>>
>>>>>>> Hi Yunda,
>>>>>>>
>>>>>>> There is a potential problem with using a ttyLocker here,
>>>>>>> depending on exactly what events are being traced and from where -
>>>>>>> you must always be in code where it is both safe to acquire the
>>>>>>> lock, and safe to block waiting for the lock if it is not available.
>>>>>>>
>>>>>>> I think I would prefer to see unlocked tracing by default with a
>>>>>>> flag to use locked-tracing if requested.
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>> On 19/04/2013 6:26 PM, 云达(Yunda) wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> I found that the output from multiple threads may be mixed
>>>>>>>> together when using EnableTracing. It happens many times in my test
>> case like this:
>>>>>>>>
>>>>>>>> Allocation outside TLAB: [Allocation in new TLAB: [Allocation in
>>>>>>>> new
>>>>>>>> TLAB: [Allocation in new TLAB: [Class = [C, Allocation in new TLAB:
>>>>>>>> [Class = [I, Allocation in new TLAB: [Java Monitor Wait: [Class =
>>>>>>>> java/lang/String, Allocation Size = 24, Allocation Size = 24,
>>>>>>>> Class = java/lang/String, Class = [I, Allocation in new TLAB:
>>>>>>>> [Class = [C, Allocation in new TLAB: [Allocation Size = 192]
>>>>>>>>
>>>>>>>> Class =
>>>>>>>> com/sun/org/apache/xerces/internal/dom/DeferredElementNSImpl,
>>>>>>>> Allocation Size = 24, Allocation Size = 24, TLAB Size = 23712280]
>>>>>>>>
>>>>>>>> TLAB Size = 24607080]
>>>>>>>>
>>>>>>>> Allocation Size = 24, Monitor Class =
>>>>>>>> java/lang/ref/Reference$Lock, TLAB Size = 25054480]
>>>>>>>>
>>>>>>>> TLAB Size = 25054480]
>>>>>>>>
>>>>>>>> Allocation in new TLAB: [Class = [CTLAB Size = 24607080]
>>>>>>>>
>>>>>>>> Allocation Size = 72, Class = [C, TLAB Size = 24159728]
>>>>>>>>
>>>>>>>> , Allocation Size = 32, TLAB Size = 23712288]
>>>>>>>>
>>>>>>>> It's very confusing and it's even not easy to tell how many
>>>>>>>> events there are. I think the reason is that the writeEvent()
>>>>>>>> method of each
>>>>>>>> Event* class output the fields of event one by one without using
>>>>>>>> any lock. So I made a small patch which add ttyLocker to
>>>>>>>> writeEvent() method and after applying this patch there's no
>>>>>>>> output mixed together in my test case(against
>>>>> http://hg.openjdk.java.net/hsx/hsx24/hotspot/):
>>>>>>>>
>>>>>>>> diff -r edd1619a3ae4 src/share/vm/trace/traceEventClasses.xsl
>>>>>>>>
>>>>>>>> --- a/src/share/vm/trace/traceEventClasses.xsl Thu Apr 18
>> 13:50:58
>>>>>>>> 2013 -0700
>>>>>>>>
>>>>>>>> +++ b/src/share/vm/trace/traceEventClasses.xsl Fri Apr 19
>>>>>>>> +++ 16:12:38
>>>>>>>> +++ 2013
>>>>>>>> +0800
>>>>>>>>
>>>>>>>> @@ -132,6 +132,7 @@
>>>>>>>>
>>>>>>>> void writeEvent(void) {
>>>>>>>>
>>>>>>>> ResourceMark rm;
>>>>>>>>
>>>>>>>> HandleMark hm;
>>>>>>>>
>>>>>>>> + ttyLocker ttyl;
>>>>>>>>
>>>>>>>> TraceStream ts(*tty);
>>>>>>>>
>>>>>>>> ts.print("<xsl:value-of select="@label"/>: [");
>>>>>>>>
>>>>>>>> <xsl:apply-templates select="value|structvalue"
>>>>>>>> mode="write-data"/>
>>>>>>>>
>>>>>>>> I searched before sending this mail I didn't find anyone who
>>>>>>>> covering thisJ
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>>
>>>>>>>> Yunda
>>>>>>>>
>>>>>>>>
>>>>>>>> -----------------------------------------------------------------
>>>>>>>> ---
>>>>>>>> --
>>>>>>>> --
>>>>>>>>
>>>>>>>> This email (including any attachments) is confidential and may be
>>>>>>>> legally privileged. If you received this email in error, please
>>>>>>>> delete it immediately and do not copy it or use it for any
>>>>>>>> purpose or disclose its contents to any other person. Thank you.
>>>>>>>>
>>>>>>>> 本电邮(包括任何附件)可能含有机密资料并受法律保护。如您不是
>> 正确
>>>>> 的
>>>>>>> 收件人，
>>>>>>>> 请您立即删除本邮件。请不要将本电邮进行复制并用作任何其他用
>> 途、
>>>>>>> 或透露本邮
>>>>>>>> 件之内容。谢谢。
>>>>>>
>>>>>> ________________________________
>>>>>>
>>>>>> This email (including any attachments) is confidential and may be
>>>>>> legally
>>>>> privileged. If you received this email in error, please delete it
>>>>> immediately and do not copy it or use it for any purpose or disclose
>>>>> its contents to any other person. Thank you.
>>>>>
>>>>>>
>>>>>> 本电邮(包括任何附件)可能含有机密资料并受法律保护。如您不是正
>> 确的
>>>>> 收件人，请您立即删除本邮件。请不要将本电邮进行复制并用作任何
>> 其他
>>>>> 用途、或透
>>>>>> 露本邮件之内容。谢谢。
>>>>>>
>>>>
>>>> ________________________________
>>>>
>>>> This email (including any attachments) is confidential and may be legally
>> privileged. If you received this email in error, please delete it
>> immediately and
>> do not copy it or use it for any purpose or disclose its contents to any
>> other
>> person. Thank you.
>>>>
>>>> 本电邮(包括任何附件)可能含有机密资料并受法律保护。如您不是正确
>> 的收件人，请您立即删除本邮件。请不要将本电邮进行复制并用作任何其
>> 他用途、或
>>>> 透露本邮件之内容。谢谢。
>>>>
>
>
> ________________________________
>
> This email (including any attachments) is confidential and may be legally
> privileged. If you received this email in error, please delete it immediately
> and do not copy it or use it for any purpose or disclose its contents to any
> other person. Thank you.
>
> 本电邮(包括任何附件)可能含有机密资料并受法律保护。如您不是正确的收件人，请您立即删除本邮件。请不要将本电邮进行复制并用作任何其他用途、或透露本邮件之内容。谢谢。
