Wireshark · Wireshark-dev: Re: [Wireshark-dev] [Wireshark-commits] rev 35393: /trunk/epan/dissectors/ /trunk/epan/dissectors/:

[Martin Mathieson <martin.r.mathieson@xxxxxxxxxxxxxx>]

On Thu, Jan 6, 2011 at 4:54 PM, Jeff Morriss <jeff.morriss.ws@gmail.com> wrote:

Anders Broman wrote:

 Martin Mathieson skrev 2011-01-06 17:03:

Thanks, I do believe that this is a special case - I wouldn't want to use tvb_get_ptr() anywhere else.

Regards,
Martin

On Thu, Jan 6, 2011 at 3:58 PM, Jeff Morriss <jeff.morriss.ws <http://jeff.morriss.ws>@gmail.com <http://gmail.com>> wrote:

   Hi Martin,

   Yeah, the code looked safe, I've just been on a mission to reduce
   the usage of tvb_get_ptr() (which I'd love to put in the category
   of "do not use!"--the only problem there being that it's used all
   over the place).  It did occur to me later that it might be
   slower;  I'll revert the change in a bit.

   Regards,
   -Jeff

   Martin Mathieson wrote:

       Jeff,
       I made the change to use tvb_get_ptr() because a profile
       showed that getting the strings each time was quite slow.
       The reason I thought this is safe is that this protocol is
       really a header written out by the corresponding wiretap
       module, so it should be well-formed (if the file being read
       isn't well-formed that will be rejected by the wiretap module).

       I can't remember how much slower it was, but it gets done
       several times for each frame read from the file.

       Best regards,
       Martin

Could tvb_get_ephemeral_stringz() be made more efficient?

I suppose that most of the work is fixed: allocating some (ep) memory, (in the case of the *z functions, finding the length of the string), then doing the memcpy().  I can't think of any meaningful optimizations.

Martin, I assume the pre-tvb_get_ptr() code here was similar to this change in that it only retrieved the string once?  (I ask since several of the strings are used multiple times.)

The diff is at http://anonsvn.wireshark.org/viewvc/trunk/epan/dissectors/packet-catapult-dct2000.c?r1=28085&r2=28183&pathrev=35393.

There were some strings that were being retrived in more than one place, but probably only once for any given frame.  I suspect the slowest part was probably the block around line 1610 where it was pulling out these 3 strings for every frame.
 

I suppose for cases like this a get_string function that uses the TVB as the backing store (and only pushes the tvb_get_strsize()+tvb_get_ptr() out of the dissector and into tvbuff.c) might be an option to help reduce the prevalence of tvb_get_ptr() in dissectors.  That may not be a bad idea because I think I saw some other dissectors doing the same thing as this one--and why take the performance hit?

My strings are null-terminated, and I was using tvb_get_ephemeral_string(), whereas I already knew the lengths from when I first parsed the line, and that they do terminate with a NULL before the end of the tvb.  Would this function do more than just cast tvb_get_ptr() to a char* ?  If not, it wouldn't be any more safe.

Martin
 

___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev@xxxxxxxxxxxxx>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
           mailto:wireshark-dev-request@xxxxxxxxxxxxx?subject=unsubscribe