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

gvstream: improve performance of receiving loop (issue #616) #617

Merged
merged 4 commits into from
Jan 12, 2022

Conversation

EmilioPeJu
Copy link
Contributor

@EmilioPeJu EmilioPeJu commented Jan 4, 2022

See issue 616

@EmmanuelP EmmanuelP marked this pull request as draft January 5, 2022 07:12
@EmilioPeJu EmilioPeJu force-pushed the recvmmsg branch 2 times, most recently from ce055db to c050654 Compare January 5, 2022 11:51
@EmilioPeJu
Copy link
Contributor Author

The socket was set to non-blocking mode to allow g_socket_receive_messages to return fewer number of messages.
Some tests with arv-fake-gv-camera were done and the improvement was significant.
Before the change:

$ arv-camera-test-0.8 -w 512 -h 512 -f 1000 --duration 5
Looking for the first available camera
vendor name            = Aravis
model name             = Fake
device serial number   = GV01
image width            = 512
image height           = 512
horizontal binning     = 1
vertical binning       = 1
exposure               = 10000 µs
gain                   = 0 dB
payload                = 262144 bytes
gv n_stream channels   = 1
gv current channel     = 0
gv packet delay        = 0 ns
gv packet size         = 1400 bytes
542 frames/s -     142 MiB/s
549 frames/s -     144 MiB/s - 1 error
552 frames/s -     145 MiB/s
500 frames/s -     131 MiB/s - 15 errors
530 frames/s -     139 MiB/s
n_completed_buffers    = 2673
n_failures             = 16
n_underruns            = 0
n_timeouts             = 0
n_aborteds             = 0
n_missing_frames       = 0
n_size_mismatch_errors = 0
n_received_packets     = 523776
n_missing_packets      = 1445
n_error_packets        = 0
n_ignored_packets      = 0
n_resend_requests      = 0
n_resent_packets       = 0
n_resend_ratio_reached = 0
n_resend_disabled      = 0
n_duplicated_packets   = 0
n_transferred_bytes    = 708415692
n_ignored_bytes        = 0

After the change:

$ arv-camera-test-0.8 -w 512 -h 512 -f 1000 --duration 5
Looking for the first available camera
vendor name            = Aravis
model name             = Fake
device serial number   = GV01
image width            = 512
image height           = 512
horizontal binning     = 1
vertical binning       = 1
exposure               = 10000 µs
gain                   = 0 dB
payload                = 262144 bytes
gv n_stream channels   = 1
gv current channel     = 0
gv packet delay        = 0 ns
gv packet size         = 1400 bytes
714 frames/s -     187 MiB/s
778 frames/s -     204 MiB/s - 5 errors
805 frames/s -     211 MiB/s
911 frames/s -     239 MiB/s
879 frames/s -     230 MiB/s
n_completed_buffers    = 4087
n_failures             = 5
n_underruns            = 0
n_timeouts             = 0
n_aborteds             = 0
n_missing_frames       = 0
n_size_mismatch_errors = 0
n_received_packets     = 797823
n_missing_packets      = 446
n_error_packets        = 0
n_ignored_packets      = 0
n_resend_requests      = 0
n_resent_packets       = 0
n_resend_ratio_reached = 0
n_resend_disabled      = 0
n_duplicated_packets   = 0
n_transferred_bytes    = 1079100544
n_ignored_bytes        = 0

@EmilioPeJu
Copy link
Contributor Author

EmilioPeJu commented Jan 5, 2022

I think it might be ready to be considered, I'll try testing it with real hardware though

@EmilioPeJu EmilioPeJu marked this pull request as ready for review January 5, 2022 12:23
@EmilioPeJu
Copy link
Contributor Author

EmilioPeJu commented Jan 5, 2022

This change was successfully tested using a camera Mako G-319B (Allied Vision), and it was confirmed to reduce the error rate.

@EmilioPeJu
Copy link
Contributor Author

EmilioPeJu commented Jan 5, 2022

I don't know why the windows tests are failing, I replicated the build environment using msys2 and the tests did not fail:

user@DESKTOP-X MSYS ~/aravis/build
$ meson test
ninja: Entering directory `C:/msys64/home/user/aravis/build'
ninja: no work to do.
1/9 aravis:main / evaluator      OK              0.09s
2/9 aravis:main / buffer         OK              0.17s
3/9 aravis:main / misc           OK              0.06s
4/9 aravis:main / dom            OK              0.08s
5/9 aravis:main / genicam        OK              0.11s
6/9 aravis:main / fake           OK              1.34s
7/9 aravis:network / fakegv      OK              9.20s
8/9 aravis:python / fake.py      OK              0.95s
9/9 aravis:python / exception.py OK              1.20s


Ok:                 9
Expected Fail:      0
Fail:               0
Unexpected Pass:    0
Skipped:            0
Timeout:            0

Full log written to C:/msys64/home/user/aravis/build/meson-logs/testlog.txt

Maybe the CI was not using the latest msys2 or maybe the fakegv test is trying to allocate more resources than the CI allows

@EmmanuelP
Copy link
Contributor

I don't know why the windows tests are failing

I don't know either. Your patch looks good, and the CI pipeline is successful on the main branch. I even tried to rebase your patch over the main branch, but the pipeline still fails.

Unfortunately, this CI failure is a real blocker. I can not leave the Windows build out of the CI. You may want to try to conditionally build without this optimization on the Windows platform.

@EmilioPeJu
Copy link
Contributor Author

I've conditionally set the number of messages because I suspect this could be a resource limit related issue, let's see what happens

@EmilioPeJu
Copy link
Contributor Author

EmilioPeJu commented Jan 6, 2022

OK, that worked: https://github.com/lyodaom/aravis/runs/4730161807?check_suite_focus=true
Though, I think we are close to the resource limit of github CI windows nodes.

@EmmanuelP
Copy link
Contributor

Great!

Could you add a build configuration option to set the number of messages, and use it for the Windows CI, instead of always define it to 1 ?

@EmmanuelP
Copy link
Contributor

As a bonus, you could try to use a sensible value for the incoming message buffer size, instead of unconditionally use 64kB.

@EmilioPeJu EmilioPeJu force-pushed the recvmmsg branch 5 times, most recently from 3f5782d to dc9e4c1 Compare January 11, 2022 23:18
@EmilioPeJu
Copy link
Contributor Author

EmilioPeJu commented Jan 11, 2022

OK, I've created a meson option called gv-buffers, I thought referring to buffers was more meaningful to user (instead of messages).
The parameter ended up in file arvparams.h because arvfeatures.h seems to be for enable/disable type of options.

Regarding the packet buffer size, it was calculated based on the GVSP packet size.

Copy link
Contributor

@EmmanuelP EmmanuelP left a comment

Choose a reason for hiding this comment

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

There is some minor issues to fix, but the patch looks fine.

I still have to find some time to test it with real hardware before I can merge.

meson_options.txt Outdated Show resolved Hide resolved
src/arvparams.h.in Outdated Show resolved Hide resolved
src/arv.h Outdated Show resolved Hide resolved
@EmmanuelP EmmanuelP added the 5. Gige Issue in the GigEVision implementation label Jan 12, 2022
The number of buffers is also the number of maximum messages that can
be received from kernel in one system call, therefore, the more expensive
system calls are, the higher this value should be
This intends to avoid reaching the memory limit in CI windows nodes
@EmmanuelP EmmanuelP merged commit b4211e5 into AravisProject:main Jan 12, 2022
@EmmanuelP
Copy link
Contributor

Thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5. Gige Issue in the GigEVision implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants