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
