[patch] Breakpoint - only reset instruction bytes covered by breakpoint instruction bytes

[patch] Breakpoint - only reset instruction bytes covered by breakpoint instruction bytes

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 505 bytes --]

Hi,

This is kind of an micro optimization. But it was noticable during a
trace while investigating breakpoint set/reset behavior. And potentially
setting bytes in the inferior is expensive. So here is a little patchlet
to not do any unnecessary work while resetting (or uninstalling) a
breakpoint.

2007-07-09  Mark Wielaard  <mwielaard@redhat.com>

        * Breakpoint.java (reset): Only copy back bytes covered by
        breakpiont instructions.

Tested on x86_64/FC6, no regressions.

Cheers,

Mark

[-- Attachment #2: Breakpoint.patch --]
[-- Type: text/x-patch, Size: 1034 bytes --]

Index: frysk-core/frysk/proc/Breakpoint.java
===================================================================
RCS file: /cvs/frysk/frysk-core/frysk/proc/Breakpoint.java,v
retrieving revision 1.12
diff -u -r1.12 Breakpoint.java
--- frysk-core/frysk/proc/Breakpoint.java	2 Jul 2007 14:40:16 -0000	1.12
+++ frysk-core/frysk/proc/Breakpoint.java	9 Jul 2007 08:15:17 -0000
@@ -172,13 +172,18 @@
    */
   private void reset(Task task)
   {
-    ByteBuffer buffer;
-    
-    buffer = task.getMemory();
+    ByteBuffer buffer = task.getMemory();
     buffer.position(address);
     
+    Isa isa = task.getIsa();
+    Instruction bpInstruction = isa.getBreakpointInstruction();
+    byte[] bp = bpInstruction.getBytes();
+
     byte[] bs = origInstruction.getBytes();
-    for (int index = 0; index < bs.length; index++)
+
+    // Only need to put back the part of the original instruction
+    // covered by the breakpoint instruction bytes.
+    for (int index = 0; index < bp.length; index++)
       buffer.putByte(bs[index]);
   }
 

Re: [patch] Breakpoint - only reset instruction bytes covered by breakpoint instruction bytes

[Andrew Cagney]

Please use the ByteBuffer's bulk get/put methods.

Andrew

Mark Wielaard wrote:
> Hi,
>
> This is kind of an micro optimization. But it was noticable during a
> trace while investigating breakpoint set/reset behavior. And potentially
> setting bytes in the inferior is expensive. So here is a little patchlet
> to not do any unnecessary work while resetting (or uninstalling) a
> breakpoint.
>
> 2007-07-09  Mark Wielaard  <mwielaard@redhat.com>
>
>         * Breakpoint.java (reset): Only copy back bytes covered by
>         breakpiont instructions.
>
> Tested on x86_64/FC6, no regressions.
>
> Cheers,
>
> Mark
>   
> ------------------------------------------------------------------------
>
> Index: frysk-core/frysk/proc/Breakpoint.java
> ===================================================================
> RCS file: /cvs/frysk/frysk-core/frysk/proc/Breakpoint.java,v
> retrieving revision 1.12
> diff -u -r1.12 Breakpoint.java
> --- frysk-core/frysk/proc/Breakpoint.java	2 Jul 2007 14:40:16 -0000	1.12
> +++ frysk-core/frysk/proc/Breakpoint.java	9 Jul 2007 08:15:17 -0000
> @@ -172,13 +172,18 @@
>     */
>    private void reset(Task task)
>    {
> -    ByteBuffer buffer;
> -    
> -    buffer = task.getMemory();
> +    ByteBuffer buffer = task.getMemory();
>      buffer.position(address);
>      
> +    Isa isa = task.getIsa();
> +    Instruction bpInstruction = isa.getBreakpointInstruction();
> +    byte[] bp = bpInstruction.getBytes();
> +
>      byte[] bs = origInstruction.getBytes();
> -    for (int index = 0; index < bs.length; index++)
> +
> +    // Only need to put back the part of the original instruction
> +    // covered by the breakpoint instruction bytes.
> +    for (int index = 0; index < bp.length; index++)
>        buffer.putByte(bs[index]);
>    }
>  
>   

Re: [patch] Breakpoint - only reset instruction bytes covered by breakpoint instruction bytes

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]

On Mon, 2007-07-09 at 09:43 -0400, Andrew Cagney wrote:
> Please use the ByteBuffer's bulk get/put methods.

Nice idea! But there is no bulk put method in ByteBuffer (there is a
bulk get(), but there are no single byte get() loops in the current
code). It would be nice to have a bulk put() method to optimize things,
so I filed bug http://sourceware.org/bugzilla/show_bug.cgi?id=4760

Cheers,

Mark

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch] Breakpoint - only reset instruction bytes covered by breakpoint instruction bytes

[Andrew Cagney]

Mark Wielaard wrote:
> On Mon, 2007-07-09 at 09:43 -0400, Andrew Cagney wrote:
>   
>> Please use the ByteBuffer's bulk get/put methods.
>>     
>
>   
Interesting slip-up, fortunately you've got peek and peekFully method 
ready to wrap?  As for micro-optimizing the PtraceByteBuffer, that is 
something we can pick up when there's evidence supporting the need.

> Nice idea! But there is no bulk put method in ByteBuffer (there is a
> bulk get(), but there are no single byte get() loops in the current
> code). It would be nice to have a bulk put() method to optimize things,
> so I filed bug http://sourceware.org/bugzilla/show_bug.cgi?id=4760
>
> Cheers,
>
> Mark
>   

Re: [patch] Breakpoint - only reset instruction bytes covered by breakpoint instruction bytes

[Chris Moller]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]



Andrew Cagney wrote:
> Mark Wielaard wrote:
>> On Mon, 2007-07-09 at 09:43 -0400, Andrew Cagney wrote:
>>  
>>> Please use the ByteBuffer's bulk get/put methods.
>>>     
>>
>>   
> Interesting slip-up, fortunately you've got peek and peekFully method
> ready to wrap?  As for micro-optimizing the PtraceByteBuffer, that is
> something we can pick up when there's evidence supporting the need.
>
>> Nice idea! But there is no bulk put method in ByteBuffer

I'm not entirely sure it's what you guys are talking about, but
frysk-imports/frysk/sys/cni/StatelessFile.cxx supports a pwrite() that's
analogous to the bulk pread(), but it was never incorporated into
MemorySpaceByteBuffer because no one seemed interested.

>> (there is a
>> bulk get(), but there are no single byte get() loops in the current
>> code). It would be nice to have a bulk put() method to optimize things,
>> so I filed bug http://sourceware.org/bugzilla/show_bug.cgi?id=4760
>>
>> Cheers,
>>
>> Mark
>>   
>

-- 
Chris Moller

  Java: the blunt scissors of programming languages.
      -- Dave Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: [patch] Breakpoint - only reset instruction bytes covered by breakpoint instruction bytes

[Andrew Cagney]

Chris Moller wrote:
> Andrew Cagney wrote:
>   
>> Mark Wielaard wrote:
>>     
>>> On Mon, 2007-07-09 at 09:43 -0400, Andrew Cagney wrote:
>>>  
>>>       
>>>> Please use the ByteBuffer's bulk get/put methods.
>>>>     
>>>>         
>>>   
>>>       
>> Interesting slip-up, fortunately you've got peek and peekFully method
>> ready to wrap?  As for micro-optimizing the PtraceByteBuffer, that is
>> something we can pick up when there's evidence supporting the need.
>>
>>     
>>> Nice idea! But there is no bulk put method in ByteBuffer
>>>       
>
>   

It is!  So just a few dots need to be connected.

> I'm not entirely sure it's what you guys are talking about, but
> frysk-imports/frysk/sys/cni/StatelessFile.cxx supports a pwrite() that's
> analogous to the bulk pread(), but it was never incorporated into
> MemorySpaceByteBuffer because no one seemed interested.
>
>   
>>> (there is a
>>> bulk get(), but there are no single byte get() loops in the current
>>> code). It would be nice to have a bulk put() method to optimize things,
>>> so I filed bug http://sourceware.org/bugzilla/show_bug.cgi?id=4760
>>>
>>> Cheers,
>>>
>>> Mark
>>>   
>>>       
>
>