-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Improve performance with nested PacketField
s
#4727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Version used for reporting of secdev#4705.
- Set step numbers. - Clarify max times. - Use regular parsing through instantiation in step 014.
- Set step numbers.
- Test configurations extracted as global constants. - Base `TestPacket` class added. Made `M`, `I` and `F` inherit from it. - Max time computed from expected number of operations. - Default instantiations ran once (not twice anymore). - Serialization test cases added to highlight cache improvements.
…4706, secdev#4707) - secdev#4705: Performance issues with nested `PacketField`s: - Cache saved in `Packet.self_build()`. - `Packet`s marked as explicit systematically in `Packet.build()` to avoid a useless clone in `do_build()`. - `Packet.do_init_cached_fields()` optimized: - `init_fields` parameter added to `Packet.init_fields()` and `do_init_cached_fields()`. - Misc: `do_init_cached_fields()` fixed for list field copies. - `Packet.prepare_cached_fields()` optimized: - Default fields not copied anymore. - `Packet.copy()` and `clone_with()` optimized with `_fast_copy()`: - Avoid instantiating *ref fields* with default instantiation. - `Packet.copy_fields_dict()` optimized with new `use_fields_already_set` parameter and reworked as an inner method to work directly on the `clone` instance. - Misc: `_T` variable type at the module level made local and renamed as `_VarDictFieldType` in `_fast_copy()`. - `Packet.setfieldval()` optimized: - New value discarded if unchanged.: - `_PacketField.m2i()` optimized: - Avoid useless copies of fields with default instance. - Then `dissect()` is used on it. - secdev#4707: [enhancement] Enable clear_cache() to spread upward in the packet tree: - `Packet.clear_cache()` reworked. - secdev#4706: Force cache reset as soon as the payload or a packet field is modified: - `Packet.clear_cache()` (reworked as per secdev#4707) called in `setfieldval()`, `delfieldval()`, `add_payload()` and `remove_payload()`. - `Packet.raw_packet_cache_fields` now useless, thus removed. - Same for `Packet._raw_packet_cache_field_value()`. - `Packet.self_build()` simplified by the way. - `Packet.no_cache` flag added to avoid setting `raw_packet_cache` when specified, used in `Packet.self_build()` and `do_dissect()`.
- `clear_cache` parameter added to `add_payload()`. - `True` by default. Set to `False` when `add_payload()` called from `do_dissect_payload()`.
- `upwards` (`False` by default) and `downwards` (`True` by default) parameters added to `clear_cache()`. - Set to `upwards=True` and `downwards=False` in `setfieldval()`, `delfieldval()`, `add_payload()` and `remove_payload()`. - Former implementation restored for `downwards=True`.
Useless now that `Packet.init_fields()` has a `for_dissect_only` parameter.
In `Packet.add_payload()` and `remove_payload()`.
…ecdev#4707) - `Packet._ensure_parent_of()` added. - Called in `do_init_cached_fields()`.
A couple of figures about the impact of the changes: 'perf_packet_fields.uts' times before changes:
After changes:
Biggest impacts on default instantiations (fewer copies) and serializations (cache optimizations). Before changes:
|
The code is probably rough for the moment. That's why I opened a draft PR. |
#: | ||
#: Useful when the packet contains auto fields depending on external content | ||
#: (parent or neighbour packets). | ||
no_cache = False # type: bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no_cache
flag comes like an extra feature.
It should probably be documented if we keep it.
@@ -118,6 +116,12 @@ class Packet( | |||
class_default_fields_ref = {} # type: Dict[Type[Packet], List[str]] | |||
class_fieldtype = {} # type: Dict[Type[Packet], Dict[str, AnyField]] # noqa: E501 | |||
|
|||
#: Set to ``True`` for classes which should not save a :attr:`raw_packet_cache`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've used Sphinx syntax to document attributes, but as far as I've seen, it's not used anywhere else.
I can change that for consistency with the rest of the code.
Let me know.
@@ -90,7 +88,7 @@ class Packet( | |||
"overload_fields", "overloaded_fields", | |||
"packetfields", | |||
"original", "explicit", "raw_packet_cache", | |||
"raw_packet_cache_fields", "_pkt", "post_transforms", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note the removal of the raw_packet_cache_fields
dictionary.
To be discussed.
self.underlayer = _underlayer | ||
self.parent = _parent | ||
if isinstance(_pkt, bytearray): | ||
_pkt = bytes(_pkt) | ||
self.original = _pkt | ||
# TODO: | ||
# Clarify the meaning for this `explicit` flag. | ||
# Now that cache is cleared as soon as the packet is modified, possibly we could get rid of it? | ||
self.explicit = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted above, the meaning of the explicit
attribute should probably be explained.
To be discussed.
if init_fields and (fname in init_fields): | ||
continue | ||
|
||
# Fix: Use `copy_field_value()` instead of just `value.copy()`, in order to duplicate list items as well in case of a list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small fix on list copies by the way.
@@ -417,27 +428,163 @@ def remove_parent(self, other): | |||
point to the list owner packet.""" | |||
self.parent = None | |||
|
|||
def _ensure_parent_of(self, val): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Method added, called once only for the moment in do_init_cached_fields()
.
But it could/should possibly be called in other cases?
Perhaps a bit of factorization?
**kargs | ||
) | ||
|
||
def _fast_copy( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this new method, I've tried to stick with former implementations of copy()
and clone_with()
, and optimize things when relevant.
But I feel like further simplifications could be done.
elif fld.islist or fld.ismutable: | ||
return _cpy(val) | ||
return None | ||
def clear_cache(self, upwards=False, downwards=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As proposed in #4707, I've kept the legacy downwards behaviour by default, and made the new upwards behaviour disabled by default.
@gpotter2 Sorry for disturbing, but I can't figure out how to run unit tests and check results. I'm on Windows, using Python 3.13.3 with venv, tox verion 4.25.0, branch master, commit 494e472. cd test && ./run_tests And even though I add
If I run 'fields.uts' or 'random.uts' individually, I can see the errors as expected. |
Draft PR for the following issues:
PacketField
s #4705clear_cache()
to spread upward in the packet tree #4707Packet.parent
references missing for packet fields with default instantiations.Initiated as a draft PR for preliminary discussions before an official delivery (not squashed yet).
To be discussed:
Packet.explicit
flag.Packet.raw_packet_cache_fields
removed.Checklist:
cd test && ./run_tests
ortox
)