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

Added queue.getFps() function and tests for this function #1229

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

Conversation

Erol444
Copy link
Member

@Erol444 Erol444 commented Feb 1, 2025

Works (as expected). I consulted o3-mini-high and they approved it already.

% ./message_queue_test
Randomness seeded to: 2163801883
===============================================================================
All tests passed (63 assertions in 18 test cases)

@Erol444 Erol444 requested a review from moratom February 1, 2025 11:05
@moratom
Copy link
Collaborator

moratom commented Feb 3, 2025

The test didn't fail before, I think this introduces a race condition in the code.

Copy link
Collaborator

@moratom moratom left a comment

Choose a reason for hiding this comment

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

Thanks!

Left a few comments about thread safety, but other than that looks good to me.

Might be worth considering a LockingQueue (the same as is used for the messages) since it's already thread safe and "battle tested".

src/pipeline/MessageQueue.cpp Outdated Show resolved Hide resolved
src/pipeline/MessageQueue.cpp Outdated Show resolved Hide resolved

// Get current time
auto now = std::chrono::steady_clock::now();
auto threshold = now - std::chrono::seconds(2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason behind the treshold? I would set a max queue size instead so it's more consistent for low and high FPS

Copy link
Member Author

Choose a reason for hiding this comment

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

@moratom well if you have script node and queue suddenly stops receiving messages, it would still report last FPS. Instead this logic removes messages older than 2sec so you get real FPS.

@Erol444 Erol444 requested a review from moratom February 3, 2025 11:59
@SzabolcsGergely
Copy link
Collaborator

How about getMessagesPerSecond instead getFPS?

@Erol444
Copy link
Member Author

Erol444 commented Feb 5, 2025

@SzabolcsGergely while it's more accurate, I think most people are used to FPS, even if it's actually Inference per second / Messages per second. Pehraps we could go with getQueueHz(), which is also quite widely used (aka hz == fps)?

@SzabolcsGergely
Copy link
Collaborator

@SzabolcsGergely while it's more accurate, I think most people are used to FPS, even if it's actually Inference per second / Messages per second. Pehraps we could go with getQueueHz(), which is also quite widely used (aka hz == fps)?

I don't see getQueueHz fit/good choice, personally.
This measurement refers to queue message rate per second.

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.

3 participants