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

Socket sources: Improvements for internal buffers #2230

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

AmmarAbouZor
Copy link
Member

This PR includes:

  • Increase the capacity of the internal buffer in socket byte sources to 1MB instead of the default value 8KB.
  • Add Checks for socket byte source to avoid filling the internal buffer if the server is sending the data with high frequency because the producer loop will call load on each iteration while some parsers will parse one item only on each iteration.
  • Call flush on consume when there is not enough writable memory.
  • Another check if the buffer is filled and return an error if we can't add all the incoming bytes into the buffer.

Copy link
Collaborator

@kruss kruss left a comment

Choose a reason for hiding this comment

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

LGTM!

Note: Probably to avoid duplication of the logic using the buffer for UDP/TCP cases would be an improvement.

@AmmarAbouZor
Copy link
Member Author

I've created two tests to demonstrate two issues:

  • TCP test where the DeqBuffer is panicking
  • UDP test prints how many bytes won't be parsed with parsers not consuming all the available bytes

@AmmarAbouZor AmmarAbouZor force-pushed the socket_sources_capcity branch from 237fed6 to 00926c9 Compare February 18, 2025 07:31
@AmmarAbouZor
Copy link
Member Author

  • TCP test is fixed to not reproduce the panic. However, this shows us that the safe functions of DeqBuffer could produce an undefined behavior if miss used.
  • UDP test is working now, but it still printing how many bytes won't be parsed
  • The ultimate fix for the cumulated bytes is to change parsers implementation to parse as possible from the provided slice of bytes and to not parse the first item only. I think This change is out of scope of this PR because it would introduce bigger changes + performance lose with the current implementation of the producer loop

* Increase the capacity of the internal buffer in socket byte sources to
  1MB instead of the default value 8KB.
* Add Checks for socket byte source to avoid filling the internal buffer
  if the server is sending the data with high frequency because the
  producer loop will call load on each iteration while some parsers
  will parse one item only on each iteration.
* Call flush on consume when there is not enough writable memory.
* Another check if the buffer is filled and return an error if we can't
  add all the incoming bytes into the buffer.
* Provide unit test to ensure socket byte sources won't fill out
  the internal buffer.
* One unit test demonstrates that parsers not consuming all the memory
  loaded into udp byte source buffer.
@AmmarAbouZor AmmarAbouZor force-pushed the socket_sources_capcity branch from 00926c9 to e8b165e Compare February 19, 2025 15:50
Copy link
Member

@marcmo marcmo left a comment

Choose a reason for hiding this comment

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

LGTM
just one comment

@AmmarAbouZor AmmarAbouZor merged commit 87c1bef into esrlabs:master Feb 19, 2025
2 checks passed
@AmmarAbouZor AmmarAbouZor deleted the socket_sources_capcity branch February 19, 2025 16:18
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.

4 participants