Re: [Wireshark-commits] rev 35393: /trunk/epan/dissectors/ /trunk/epan/dissectors/: packet-catapult-dct2000.c

[Martin Mathieson <martin.r.mathieson () googlemail com>]

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 () wireshark org>
> Archives:    http://www.wireshark.org/lists/wireshark-dev
> Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
>            mailto:wireshark-dev-request () wireshark org?subject=unsubscribe
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe