Skip to content
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

CANParser: expose MessageState to Cython to eliminate data copying #1382

Merged
merged 5 commits into from
Nov 2, 2024

Conversation

deanlee
Copy link
Contributor

@deanlee deanlee commented Oct 16, 2024

This PR greatly simplifies the update logic, resulting in reduced code complexity and eliminating unnecessary data copying.

Main Changes:

  1. Removal of SignalValue Struct and CANParser::query_latest Function: These components, previously used for copying updated values to Cython, have been removed. This change streamlines the data flow between C++ and Cython.
  2. Direct Exposure of MessageState to Cython: MessageState is now directly accessible from Cython, allowing values to be read straight from the C++ object. This eliminates the need for intermediate copying, optimizing performance and simplifying the overall code structure.

Additionally, this PR includes changes from #1381, where value dictionaries are now initialized directly from DBC messages, though it does not include the test_parser_empty_list() function.

Below are the performance benchmark results comparing this PR against the current master branch:

Benchmark (PR):

6000 CAN packets, 10 runs
272.64 mean ms, 278.98 max ms, 268.80 min ms, 2.98 std ms
0.0454 mean ms / CAN packet

Benchmark (Master):

6000 CAN packets, 10 runs
335.81 mean ms, 394.27 max ms, 312.58 min ms, 24.96 std ms
0.0560 mean ms / CAN packe

Most of the time is spent converting and copying messages from Python to C++. so while the current results don't show a massive improvement, after #1383 is merged, this PR could reduce the time per packet from 0.0318 ms to under 0.02 ms. I will run additional benchmarks to confirm once that change is integrated.

@github-actions github-actions bot added the can related to CAN tools, aka opendbc/can/ label Oct 16, 2024
@deanlee deanlee mentioned this pull request Oct 16, 2024
@deanlee deanlee force-pushed the parser_remove_query_latest branch from be80445 to b4557e3 Compare October 16, 2024 15:57
@deanlee deanlee force-pushed the parser_remove_query_latest branch from b4557e3 to 1fa97ff Compare November 1, 2024 06:11
@deanlee deanlee mentioned this pull request Nov 1, 2024
@sshane sshane force-pushed the parser_remove_query_latest branch from eef22f6 to 3b3f351 Compare November 2, 2024 00:50
@sshane
Copy link
Contributor

sshane commented Nov 2, 2024

Benchmarks on a 3X:

PR:

6000 CAN packets, 10 runs
395.90 mean ms, 397.14 max ms, 394.55 min ms, 0.83 std ms
0.0660 mean ms / CAN packet

master (after bus/addr filter PR):

6000 CAN packets, 10 runs
437.80 mean ms, 440.19 max ms, 436.00 min ms, 1.38 std ms
0.0730 mean ms / CAN packet

@sshane sshane force-pushed the parser_remove_query_latest branch from 3b3f351 to 1fa97ff Compare November 2, 2024 04:13
@deanlee
Copy link
Contributor Author

deanlee commented Nov 2, 2024

@sshane : FYI, this PR also resolves issue #1066, as it no longer relies on timestamps for updating the last values. Instead, it updates the updated_addresses set whenever a CAN signal is successfully parsed.

if (state_it->second.parse(can.nanos, frame.dat)) {
    updated_addresses.insert(state_it->first);
}

This approach ensures the updated values are tracked more reliably, independent of the timestamp.

I believe we can close issue #1066 once this is merged.

@sshane
Copy link
Contributor

sshane commented Nov 2, 2024

Nice clean up!

@sshane sshane added the cleanup label Nov 2, 2024
@sshane sshane merged commit 788a2db into commaai:master Nov 2, 2024
4 checks passed
@deanlee deanlee deleted the parser_remove_query_latest branch November 2, 2024 04:42
@sshane
Copy link
Contributor

sshane commented Nov 23, 2024

@deanlee do you mind making the same change with PlotJuggler? It used SignalValue which we removed

@deanlee
Copy link
Contributor Author

deanlee commented Nov 23, 2024

@sshane : commaai/PlotJuggler#67 should work, However, since I'm still unable to build PlotJuggler, feel free to take over it if you'd like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can related to CAN tools, aka opendbc/can/ cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants