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

Improve performance for endian read/write functions #3

Merged
merged 5 commits into from
Mar 23, 2020
Merged

Improve performance for endian read/write functions #3

merged 5 commits into from
Mar 23, 2020

Conversation

armeenm
Copy link

@armeenm armeenm commented Mar 23, 2020

Functions which accept endianness as a template parameter can benefit from constexpr branching to determine if byteswapping should take place or not. I benchmarked using the writelog example. Before:

500k double append (test2.log): 44470
50 string append (test-string.log): 262
Double array append (test-double-array.log): 6223
String array append (test-string-array.log): 330

After:

500k double append (test2.log): 38896
50 string append (test-string.log): 257
Double array append (test-double-array.log): 6170
String array append (test-string-array.log): 257

These results are fairly consistent across runs (generally a ~6ms improvement for the 500k double append).

@PeterJohnson
Copy link
Owner

Please separately PR the endian templated byte_swap change to the main wpilibsuite repo, since it could be committed independently.

Regarding the macros, we'll need to see if this fails when run against CI, in particular on Mac.

@PeterJohnson
Copy link
Owner

The other thing that would be great to have is real unit tests (e.g. using GoogleTest). We might need to do some work to enable use of temporary filenames though.

@PeterJohnson PeterJohnson merged commit cb16c9a into PeterJohnson:datalog Mar 23, 2020
@armeenm
Copy link
Author

armeenm commented Mar 28, 2020

I just realized the endian stuff is from LLVM. Do we still want to make changes to that?

@PeterJohnson
Copy link
Owner

I'm okay with making a change there; we don't want to do dramatic things like reformatting code, but minor changes I can deal with when merging from upstream 1-2 times a year.

@PeterJohnson
Copy link
Owner

It might be good to PR that upstream as well though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants