opendbc icon indicating copy to clipboard operation
opendbc copied to clipboard

Refactor CAN Parser to boost performance

Open deanlee opened this issue 1 year ago • 5 comments

Shift most logic to C++, read values directly from c++ MessageState instead of maintaining a copy in cython. Cython's update_string acts as a simple interface wrapper for the C++ function.


def update_strings(self, strings, sendcan=False):
    return self.can.update_strings(strings, sendcan)

master:

6000 CAN packets, 20 runs 293.75 mean ms, 295.81 max ms, 289.66 min ms, 1.42 std ms 0.0490 mean ms / CAN packet

this branch:

6000 CAN packets, 20 runs 29.34 mean ms, 33.17 max ms, 27.16 min ms, 1.29 std ms 0.0049 mean ms / CAN packet

This PR also resolves https://github.com/commaai/opendbc/issues/913. Since we no longer rely on calling update_strings in __init__ to initialize Cython members, update_strings() now returns an empty list if strings is empty.

After this PR, we can simplify car/interface by using only one CanParser. With a single call to update_string for parsing all messages, there's no need to repeatedly feed identical can_strings to multiple instances of CanParser's update_string method for message filtering. This straightforward approach greatly enhances car/interface and parsing efficiency.

Additional Notes:

PlotJuggler will be broken after this PR because data is now read directly from MessageState instead of being copied using query_latest. However, it is quite easy to modify PlotJuggler to use the new API.

deanlee avatar May 26 '24 21:05 deanlee

:eyes:

batman@workstation-shane:~/openpilot/selfdrive/debug$ ./check_can_parser_performance.py 
100%|██████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 10/10 [00:01<00:00,  7.92it/s]
6000 CAN packets, 10 runs
99.36 mean ms, 99.86 max ms, 98.82 min ms, 0.33 std ms
0.0166 mean ms / CAN packet
batman@workstation-shane:~/openpilot/selfdrive/debug$ ./check_can_parser_performance.py 
100%|██████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 10/10 [00:00<00:00, 20.73it/s]
6000 CAN packets, 10 runs
23.19 mean ms, 23.77 max ms, 22.84 min ms, 0.25 std ms
0.0039 mean ms / CAN packet

sshane avatar Jun 06 '24 06:06 sshane

👀

batman@workstation-shane:~/openpilot/selfdrive/debug$ ./check_can_parser_performance.py 
100%|██████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 10/10 [00:01<00:00,  7.92it/s]
6000 CAN packets, 10 runs
99.36 mean ms, 99.86 max ms, 98.82 min ms, 0.33 std ms
0.0166 mean ms / CAN packet
batman@workstation-shane:~/openpilot/selfdrive/debug$ ./check_can_parser_performance.py 
100%|██████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 10/10 [00:00<00:00, 20.73it/s]
6000 CAN packets, 10 runs
23.19 mean ms, 23.77 max ms, 22.84 min ms, 0.25 std ms
0.0039 mean ms / CAN packet

Yes, this test should be accurate. The test results from my master version are outdated.

deanlee avatar Jun 06 '24 06:06 deanlee

After this PR, we can simplify car/interface by using only one CanParser. With a single call to update_string for parsing all messages, there's no need to repeatedly feed identical can_strings to multiple instances of CanParser's update_string method for message filtering. This straightforward approach greatly enhances car/interface and parsing efficiency.

Nice, you mean we can pass in a dictionary with bus as the key, or similar?

sshane avatar Jun 08 '24 03:06 sshane

On a comma 3X:

6000 CAN packets, 10 runs
300.33 mean ms, 302.04 max ms, 296.67 min ms, 1.42 std ms
0.0501 mean ms / CAN packet

vs.

6000 CAN packets, 10 runs
86.96 mean ms, 87.91 max ms, 86.36 min ms, 0.54 std ms
0.0145 mean ms / CAN packet

sshane avatar Jun 08 '24 04:06 sshane

After this PR, we can simplify car/interface by using only one CanParser. With a single call to update_string for parsing all messages, there's no need to repeatedly feed identical can_strings to multiple instances of CanParser's update_string method for message filtering. This straightforward approach greatly enhances car/interface and parsing efficiency.

Nice, you mean we can pass in a dictionary with bus as the key, or similar?

y, or may something like {bus: [(msg1, freq1), (msg2, freq2)...], bus2: [...]}

deanlee avatar Jun 08 '24 04:06 deanlee

Thanks for contributing to opendbc! In order for us to review your PR as quickly as possible, check the following:

  • Convert your PR to a draft unless it's ready to review
  • Read the contributing docs
  • Before marking as "ready for review", ensure:
    • the goal is clearly stated in the description
    • all the tests are passing
    • include a route or your device' dongle ID if relevant

github-actions[bot] avatar Aug 20 '24 13:08 github-actions[bot]

This PR is too complicated. Closed. Some functions have been moved to PR #1382.

deanlee avatar Oct 16 '24 15:10 deanlee