search

ethz-asl / ethzasl_xsens_driver

Driver for xsens IMUs
BSD 2-Clause "Simplified" License
102 stars 112 forks source link

[WIP] No local handler functions #105

Closed moooeeeep closed 5 years ago

moooeeeep commented 5 years ago

Attempting to address https://github.com/ethz-asl/ethzasl_xsens_driver/issues/103 here.

Disclaimer: Static code analysis seems promising. I didn't test it yet. Will give notice, when I come to do this later.

The first commit also fixed some errors. As I see this, these errors were not an issue before. The local functions were defined as closures, and the parent scope also used the variable name o for the same object, that was passed then into the function under a different name.

fcolas commented 5 years ago

@moooeeeep Many thanks for your contribution. It looks good but I'll try to have a deeper look and test it in the next few days.

fcolas commented 5 years ago

@moooeeeep I've merged some support for gen5 devices but then there are conflicts. I've also tried to quickly test your branch but didn't find any significant improvement. On the contrary the load seemed very slightly higher with your version (I don't know why). Can you test on your side?

moooeeeep commented 5 years ago

That might actually be the case (and I actually should have done some research in beforehand).

According to these references, the overhead for using local functions is not so big after all (bytecode seems to be re-used), and the overhead of additional lookups to member functions might very well outweigh the benefit of not having to recreate some local functions:

Still I would like to do a little profiling myself, when I find the time.

fcolas commented 5 years ago

Alright, that'd be great. I'll change the name to add the [WIP] tag. Don't hesitate to remove it when it is ready again.

moooeeeep commented 5 years ago

I too found no improvement in performance using this approach. As I don't think I will follow up on this, I think we can safely close this PR.