Wireshark · Ethereal-dev: [Ethereal-dev] Re: Improving filter speed

[ronnie sahlberg <ronniesahlberg@xxxxxxxxx>]

Ok,

Can you
1, change the new variable infilter to a better name   like
is_referenced  or something
2, change the proto_tree_is_null() function to only take one
parameter, the protocol hf_field and return a gboolean TRUE/FALSE and
rename it as  is_protocol_referenced() or something, and only take the
int  proto hf field as parameter so one would write code like
    if(!is_protocol_referenced(proto_nfs)){
	tree=NULL
}

so one can also solve the problem with NLM and similar which calls
dissectors from other protocols and make nfs do
if( !is_protocol_referenced(proto_nfs)
  &&!is_protocol_referenced(proto_nlm)){
	tree=NULL;
}
3, document the new variable, the assumptions and howto use the new
optimization for dissectors so others can add it to the other
dissectors.
What values does it take for fields and protocols etc.


Then i will test it in a few days and check it in if
1, it seems to work
2, no one else has any objections (please comment, this can be a
potentially useful and powerful in optimizing runtime speed of
ethereal)




On Mon, 21 Mar 2005 15:35:23 +0000, didier <dgautheron@xxxxxxxx> wrote:
> ronnie sahlberg wrote:
> > "and from a vgprof output most of the time (ok it's the instruction count
> > but ethereal is so hard  on the cache that it's not a too bad proxy) is
> > in building and destroying tree nodes."
> > 
> > In that case, maybe something worth testing would be to test this
> approach:
> > 1, add a new field header_field_info.ref_count that keeps track of how
> > many times
> > a field is referenced from a filter of some sort. (similar to your patch)
> > 0 means  it is not referenced at all.
> > 2, add code in proto_tree_add_item() and friends to test if ref_count
> > is zero or not.
> > If tree->visible==0 and ref_count is zero, then proto_tree_add_item()
> > and friends just return NULL without doing anything.
> > 
> > This would then make all expensive proto_tree_add_...() functions
> > return almost immediately unless
> > 1, the tree is visible and to be displayed
> > 2, tethereal is run with the -V option
> > 3, the field is referenced by a filter and thus have to be evaluated
> > and put in the edt tree.
> > 
> > Then all dissectors would get a speedup without modifying them.
> I'm little afraid to change the behavior of all protocols.
> > 
> > Would that be a good first step in optimizing performance?
> In practice it doesn't work.
> 1) it would only work for dissectors with no fields in the filter, the 
> same can be done better with your next idea
> 2) there're dissectors which use the result of add_item without checking 
> for NULL.
> 
> > 
> > 
> > 
> > As a second step one can later add code that keeps track of whether 
> > any fields within that protocol is referenced or not by any filter and if
> not
> > then one can let the call_dissector_...() and friends check : are any
> > fields for this dissector
> > referenced by a filter or not, and if not, then just call the next
> > dissector with tree==NULL
> > thus again getting the same effect as your patch but without adding
> > the code explicitely to reset tree to NULL in the dissector itself.
> > This might require a new protocol registration routine to indicate
> > which protocols this is safe to do for (==protocols verified to not
> > call any functions from other protocols directly)
> 
> Yes it would help for 'leaf' protocols but the unsafe are the protocols 
> you want to optimize the most :(
> 
> BTW is the tree visible attribute a 'public' one for dissectors? Some 
> protocols do something like:
> if (tree) {
>  for () {
>   proto_add_txt(...., "", slow_function());
>  }
> }
> 
> lot faster:
> if (tree) {
>  if (tree->visible) for () {
>   proto_add_txt(...., "", slow_function());
>  }
> }
> 
> val_to_str(), match_strval() are slow functions, unsorted linked lists
> > 
> > 
> > On Mon, 21 Mar 2005 06:24:51 +0000, didier <dgautheron@xxxxxxxx> wrote:
> > 
> >>ronnie sahlberg wrote:
> >>
> >>>Interesting.
> >>>
> >>>Four comments:
> >>>
> >>>1, One would have to be a bit careful with where one uses
> >>>proto_tree_is_null() since there are instances where one protocol
> >>>dissector calls functions/subdissectors from other protocols.  For
> >>>example  NLM will call the dissector for NFS filehandles from the NFS
> >>>dissector. This would need to be documented. Can you write a README
> >>>for discussion on this?
> >>
> >>I'll look at it.
> >>
> >>>2, proto_tree_is_null() would need a better more appropriate name.
> >>
> >>:)
> >>
> >>>3, comment in your patch: i really think you need to do refcounting
> >>>for these items. I am not sure, but one might do refcounting just to
> >>>be on the safe side.
> >>>The refcounting might be as easy to implement as just
> >>>incrementing/decrementing hfinfo->infilter and letting ==0 mean not
> >>>used at all in any filter.  Would need to check that that would work.
> >>
> >>infilter is a tristate -1 is used for filtering on protocol type. Could
> >>use some ugly bitfield though
> >>
> >>>4, proto_tree_is_null() is a too blunt instrument, it either enables
> >>>all fields or none fields. In addition to that one might also consider
> >>>putting
> >>>if(!( ->visible or ->infilter ))return
> >>>at the beginning of the proto_tree_add_ functions as well to further
> >>>reduce the number of calls made.
> >>>This might reduce the filter overhead even further.
> >>
> >>Yes but:
> >>1) it will affect all dissectors, no idea if it's good or bad
> >>2) not sure about tap stuff.
> >>3) you can't do it at the beginning because then you return a NULL
> >>pointer and it breaks stuff like
> >>     ti = proto_tree_add_item(tree, hf_val1 ...
> >>     /* ti is NULL */
> >>     s_tree = proto_item_add_subtree(ti, ett);
> >>     /* s_tree  is NULL */
> >>      proto_tree_add_uint_hidden(s_tree, hf_val2...
> >>     /* oops if you need  hf_val2*/
> >>
> >>and from a vgprof output most of the time (ok it's the instruction count
> >>but ethereal is so hard  on the cache that it's not a too bad proxy) is
> >>in building and destroying tree nodes.
> >>
> >>Didier
> >>
> >>
> > 
> > 
> > 
> 
> 
>