Improve performance with nested `PacketField`s

[alxroyer-thales]

Draft PR for the following issues:

• #4705
• #4706
• #4707
• #4735

Initiated as a draft PR for preliminary discussions before an official delivery (not squashed yet).

To be discussed:

• Changes currently based on v2.6.1 => shall I align on master? or useful to deliver a fix for a version before?
• Packet.explicit flag.
• Packet.raw_packet_cache_fields removed.

Checklist:

• [x] If you are new to Scapy: I have checked CONTRIBUTING.md (esp. section submitting-pull-requests)
• [ ] I squashed commits belonging together
• [x] I added unit tests or explained why they are not relevant
• [ ] I executed the regression tests (using cd test && ./run_tests or tox)
• [x] If the PR is still not finished, please create a Draft Pull Request

[alxroyer-thales]

A couple of figures about the impact of the changes:

'perf_packet_fields.uts' times before changes:

passed 2E618233 000.02s 000) Define `NUMBER_OF_I_PER_M` and `NUMBER_OF_F_PER_I` constants, and prepare a base `TestPacket` class.
passed 9DC21D20 000.00s 001) Define the `F` final packet class.
passed B7BBB574 000.01s 002) Define the `I` intermediate packet class.
passed E12AE3F0 000.54s 003) Define the `M` main packet class.
passed 10E80BEA 000.00s 004) Build a default instance of `F`.
passed 59DBEF73 000.01s 005) Build a default instance of `I`.
failed B573553A 002.91s 006) Build a default instance of `M`.
failed A6E98C89 005.31s 007) Launch serialization from the latest instance of `M` created.
failed 4DAB5BC5 005.06s 008) Launch serialization again from the same instance of `M`.
failed 46AD5D98 005.21s 009) Update one `F` from the same instance of `M` and launch serialization again.
passed B8CC085E 001.28s 010) Parse the buffer serialized before.

After changes:

passed 2E618233 000.02s 000) Define `NUMBER_OF_I_PER_M` and `NUMBER_OF_F_PER_I` constants, and prepare a base `TestPacket` class.
passed 9DC21D20 000.00s 001) Define the `F` final packet class.
passed B7BBB574 000.01s 002) Define the `I` intermediate packet class.
passed E12AE3F0 000.50s 003) Define the `M` main packet class.
passed 10E80BEA 000.00s 004) Build a default instance of `F`.
passed 59DBEF73 000.00s 005) Build a default instance of `I`.
passed B573553A 000.53s 006) Build a default instance of `M`.
passed A6E98C89 000.13s 007) Launch serialization from the latest instance of `M` created.
passed 4DAB5BC5 000.00s 008) Launch serialization again from the same instance of `M`.
passed 46AD5D98 000.00s 009) Update one `F` from the same instance of `M` and launch serialization again.
passed B8CC085E 001.22s 010) Parse the buffer serialized before.

Biggest impacts on default instantiations (fewer copies) and serializations (cache optimizations). (It seems parsing had already been optimized with version 2.5.0.)

Before changes:

######
## Default instantiations
######


###(006)=[failed] 006) Build a default instance of `M`.

>>> TestPacket.begin_case()
>>> m = M()
>>> TestPacket.end_case(
...     init_m=1,
...     init_i=NUMBER_OF_I_PER_M,
...     init_f=NUMBER_OF_I_PER_M * NUMBER_OF_F_PER_I,
... )
Number of M inits: 1 (expected: 1)
Number of I inits: 200 (expected: 100)
Number of F inits: 60000 (expected: 10000)
Number of M builds: 0 (expected: 0)
Number of I builds: 0 (expected: 0)
Number of F builds: 0 (expected: 0)
Total number of operations expected: 10101
Execution time: 2.912 seconds (max: 1.667)
Traceback (most recent call last):
  File "<input>", line 5, in <module>
  File "<input>", line 45, in end_case
AssertionError: Execution time FAILED: 2.912 > 1.667


######
## Serialization
######


###(007)=[failed] 007) Launch serialization from the latest instance of `M` created.

>>> TestPacket.begin_case()
>>> buf = m.build()
>>> TestPacket.end_case(
...     build_m=1,
...     build_i=NUMBER_OF_I_PER_M,
...     build_f=NUMBER_OF_I_PER_M * NUMBER_OF_F_PER_I,
... )
Number of M inits: 1 (expected: 0)
Number of I inits: 300 (expected: 0)
Number of F inits: 90000 (expected: 0)
Number of M builds: 1 (expected: 1)
Number of I builds: 100 (expected: 100)
Number of F builds: 10000 (expected: 10000)
Total number of operations expected: 10101
Execution time: 5.307 seconds (max: 1.667)
Traceback (most recent call last):
  File "<input>", line 5, in <module>
  File "<input>", line 45, in end_case
AssertionError: Execution time FAILED: 5.307 > 1.667


###(008)=[failed] 008) Launch serialization again from the same instance of `M`.

>>> TestPacket.begin_case()
>>> buf = m.build()
>>> TestPacket.end_case(
...     build_m=1,  # `m` is cached, `build()` not spreaded on `i{n}` fields (thus, no `f{n}`).
...     build_i=0,
...     build_f=0,
... )
Number of M inits: 1 (expected: 0)
Number of I inits: 300 (expected: 0)
Number of F inits: 90000 (expected: 0)
Number of M builds: 1 (expected: 1)
Number of I builds: 100 (expected: 0)
Number of F builds: 10000 (expected: 0)
Total number of operations expected: 1
Execution time: 5.055 seconds (max: 0.010)
Traceback (most recent call last):
  File "<input>", line 5, in <module>
  File "<input>", line 45, in end_case
AssertionError: Execution time FAILED: 5.055 > 0.010


###(009)=[failed] 009) Update one `F` from the same instance of `M` and launch serialization again.

>>> m.i0.f0.value += 1
>>> 
>>> TestPacket.begin_case()
>>> buf = m.build()
>>> TestPacket.end_case(
...     build_m=1,
...     build_i=NUMBER_OF_I_PER_M,  # `i0` gets rebuilts, next `i{n}` fields are just asked for their cache.
...     build_f=1 * NUMBER_OF_F_PER_I,  # `i0.f0` gets rebuilt, next `i0.f{n}` fields are just asked for their cache.
... )
Number of M inits: 1 (expected: 0)
Number of I inits: 300 (expected: 0)
Number of F inits: 90000 (expected: 0)
Number of M builds: 1 (expected: 1)
Number of I builds: 100 (expected: 100)
Number of F builds: 10000 (expected: 100)
Total number of operations expected: 201
Execution time: 5.207 seconds (max: 0.033)
Traceback (most recent call last):
  File "<input>", line 5, in <module>
  File "<input>", line 45, in end_case
AssertionError: Execution time FAILED: 5.207 > 0.033

[alxroyer-thales]

I've also tried to see how things behave when making the two constants NUMBER_OF_I_PER_M and NUMBER_OF_F_PER_I vary.

In red, before the changes, in blue, after the changes:

With that angle, we better see how the blue surface remains below the red one while NUMBER_OF_I_PER_M and NUMBER_OF_F_PER_I rise up.

[alxroyer-thales]

The code is probably rough for the moment. That's why I opened a draft PR. I'll be glad to read through your remarks to make it better.

[alxroyer-thales]

@gpotter2 Sorry for disturbing, but I can't figure out how to run unit tests and check results.

I'm on Windows/Git Bash, using Python 3.13.3 with venv, tox verion 4.25.0, branch master, commit 494e472. I do:

cd test && ./run_tests

And even though I add assert False lines in tests like 'fields.uts' or 'random.uts', I keep on getting lines at the end of 'run_tests' executions like below:

  py313--non_root: OK (12.84 seconds)
  congratulations :) (15.69 seconds)

If I run 'fields.uts' or 'random.uts' individually, I can see the errors as expected. What am I doing wrong with the 'run_tests' script?

[polybassa]

Thanks for the PR.

[polybassa]

Let's see, how much it breaks ;-)

[polybassa]

Hi, could you please fix the unit tests for your proposal. After that, I would go into a deeper review.

[alxroyer-thales]

Hi, could you please fix the unit tests for your proposal. After that, I would go into a deeper review.

I'm on it.

I've written this unittests-manual-utscapy.yml workflow file in order to be able to launch a single utscapy configuration from the matrix in 'unittests.yml'.

Good idea? or was there a simpler way to do it?

[polybassa]

Hi, could you please fix the unit tests for your proposal. After that, I would go into a deeper review.

I'm on it.

I've written this unittests-manual-utscapy.yml workflow file in order to be able to launch a single utscapy configuration from the matrix in 'unittests.yml'.

Good idea? or was there a simpler way to do it?

Usually the easiest way is to use "tox" to run the unit tests

[guedou]

Thanks for the PR!

We just discussed this internally with the Scapy maintainers team. This PR brings a lot of changes and it makes it difficult to review and make the CI pass.

Is there a possibility to break it down into smaller relevant changes?

[alxroyer-thales]

Hello all,

This PR brings a lot of changes

Indeed, it was quite a challenge, but we had to go through it for our project, otherwise a packet with lists of 100-1000 subpackets didn't build or dissect in a reasonable time.

it makes it difficult to (...) make the CI pass.

I know, I'm on it. It takes a bit of time, but I'm working it out, step by step.

difficult to review

I understand. Actually, I was counting on you to challenge me, in order to make the code smarter whenever possible. That may lead us to a bit of refactoring, no problem. Tell me.

Is there a possibility to break it down into smaller relevant changes?

Thinking about it twice, I'd say there are two parts in this PR to cover #4705:

1. Lower the number of packet instantiations while instantiating, building and dissecting packets.
2. Once the first part is done, save the results of builds with raw_packet_cache so that we don't build the packet again if it has not changed. This second part involves #4706 and #4707.

But I'm not so sure that splitting the two parts would make the things easier for the review. I can't tell either whether the 1st part alone would be enough to solve the initial performance issue.

---

In short, this PR is quite challenging yes, but worth for performance concerns. In order to lower regression risks, what about delivering it with a next major version?