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: optimize data handling by eliminating data copy after Update() #1439

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

deanlee
Copy link
Contributor

@deanlee deanlee commented Nov 3, 2024

This pull request removes unnecessary data copying from MessageState to dictionaries following update() calls. By encapsulating MessageState within a Cython object and implementing a read-only dictionary for direct access to the signal data, we eliminate the need to clear vl_all and copy updated data to vl, vl_all and ts_nanos. This optimization yields approximately a 1x performance improvement on the 3X device.

With this refactor, the impact on dictionary read performance is negligible, as the read-only dictionary is a minimal wrapper around the C++ MessageState, allowing direct access to data from the C++ unordered_map without additional overhead.

Passes tests on the device and OpenPilot branch #33926.

Key Changes:

  1. Encapsulated MessageState in a Cython Class: Enables direct access to signal data from the C++ MessageState, eliminating the need for data copying after update().
  2. Implemented a Custom ReadOnlyDict: Overrides __getitem__ to retrieve data from C++ MessageState, ensuring read-only access.
  3. Adjusted test_checksum.py: Removed deep copy requirements as the new implementation is read-only.

Below is the benchmark comparison on the 3X device:

Benchmark (PR):


6000 CAN packets, 10 runs
265.55 mean ms, 267.66 max ms, 264.63 min ms, 0.86 std ms
0.0443 mean ms / CAN packet

Benchmark (Master):

6000 CAN packets, 10 runs
491.51 mean ms, 492.63 max ms, 490.43 min ms, 0.71 std ms
0.0819 mean ms / CAN packet

This PR is an improved version of #1046

@github-actions github-actions bot added the can related to CAN tools, aka opendbc/can/ label Nov 3, 2024
@deanlee deanlee force-pushed the refactor_can_parser branch 2 times, most recently from df2318a to d43298f Compare November 3, 2024 20:42
@deanlee deanlee marked this pull request as ready for review November 3, 2024 21:02
@deanlee
Copy link
Contributor Author

deanlee commented Nov 3, 2024

After this, only one bottleneck remains: the conversion related to can_capnp_to_can_list_cpp. By using a NumPy array, the existing multi-step conversion:

capnp string -> vector[CanData] -> Python list -> vector[CanData]

can be streamlined into a single step:

capnp string -> NumPy array.

This would allow CanParser updates to be executed almost entirely in C++ code, with Cython serving as a simple wrapper for the C++ function, as shown below:

def update_strings(self, np.ndarray[uint8_t, ndim=1, mode="c"] byte_array, sendcan=False):
   cdef uint8_t[::1] arr_memview = byte_array
   return self.can.update(&arr_memview[0], byte_array.shape[0])
   

In my local draft, this optimization has the potential to double performance, reducing the processing time from 0.0443 ms per CAN packet to 0.0202 ms per CAN packet.

It’s important to note that using a NumPy array is just one option. Other alternatives may also be worth exploring.

Comment on lines 53 to 65
class ValueDict(ReadonlyDict):
def __getitem__(self, key):
return self.state.value(key)


class NanosDict(ReadonlyDict):
def __getitem__(self, key):
return self.state.ts_nanos(key)


class AllValueDict(ReadonlyDict):
def __getitem__(self, key):
return self.state.all_values(key)
Copy link
Contributor

@sshane sshane Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way we can get by with ReadonlyDict?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about ebf4696

@sshane
Copy link
Contributor

sshane commented Nov 5, 2024

This brings down Bronco card CPU usage from 28% to 21%!

@deanlee
Copy link
Contributor Author

deanlee commented Nov 7, 2024

This brings down Bronco card CPU usage from 28% to 21%!

by the way, I've also eliminate the multiple conversions from capnp can data to vector[CanFrame] while call CanParser.update. I'll test and submit soon. With this PR and the conversion simplification, CanParser and can_capnp_to_list will run entirely in C++ at full speed—extremely fast!

[update]: submitted: #1452

@deanlee deanlee force-pushed the refactor_can_parser branch 3 times, most recently from 0c5341b to 68313dd Compare November 15, 2024 05:21
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/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants