Skip to content

Conversation

@LinusJungemann
Copy link
Member

No description provided.

@LinusJungemann LinusJungemann requested a review from Copilot July 14, 2025 12:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces support for asynchronous inference by adding dynamic queue implementations and callback/drain mechanisms, and updates tests and benchmarks accordingly.

  • Added DynamicSPSCQueue and replaced static queue usage in asynchronous buffer wrappers.
  • Extended DeviceHandler, Accelerator, and BaseDriver APIs with registerCallback and drain methods.
  • Updated integration tests and benchmarks to remove fixed sleeps and rely on blocking or callback-driven result retrieval.

Reviewed Changes

Copilot reviewed 22 out of 23 changed files in this pull request and generated 4 comments.

File Description
unittests/utils/SPSCQueueTest.cpp Replaced range-insert with manual push_back loop
src/FINNCppDriver/utils/FPGAReset.hpp Added unit-test bypass for reset and included chrono literals in header
src/FINNCppDriver/core/BaseDriver.hpp Exposed registerCallback and drain APIs for async inference
integrationtest/AsyncInference.cpp Removed sleeps and switched to blocking getResults()
Comments suppressed due to low confidence (1)

unittests/utils/SPSCQueueTest.cpp:514

  • [nitpick] Instead of manually looping and calling push_back, you can use all_consumed.insert(all_consumed.end(), results.begin(), results.begin() + dequeued) for clearer intent and potentially better performance.
                for (size_t i = 0; i < dequeued; i++) {

#include <thread>
#include <vector>

using namespace std::chrono_literals;
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

[nitpick] Avoid using namespace directives in headers as they pollute the global namespace for all translation units. Consider qualifying chrono literals where needed or moving the using directive into the implementation file.

Suggested change
using namespace std::chrono_literals;
// Removed `using namespace std::chrono_literals;` to avoid namespace pollution.

Copilot uses AI. Check for mistakes.
/**
* @brief Register a callback function to be called when the inference of a batch is finished
*/
template<typename = std::enable_if<!SynchronousInference>>
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

[nitpick] This use of std::enable_if does not actually disable the template when the condition is false. To constrain these methods properly, use std::enable_if_t or C++20 concepts (requires) so the overload is SFINAE-friendly.

Suggested change
template<typename = std::enable_if<!SynchronousInference>>
template<typename = std::enable_if_t<!SynchronousInference>>

Copilot uses AI. Check for mistakes.
FINN_LOG(loglevel::info) << "Destructing Asynchronous input buffer";
workerThread.request_stop(); // Joining will be handled automatically by destruction
};
~AsyncDeviceInputBuffer() override { FINN_LOG(loglevel::info) << "Destructing Asynchronous input buffer" << std::endl; };
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

The destructor no longer calls queue.shutdown(), so the worker thread may attempt to enqueue after it's signaled to stop, causing potential data races or hangs. Ensure the queue is shut down before thread teardown (e.g., call queue.shutdown() before logging).

Suggested change
~AsyncDeviceInputBuffer() override { FINN_LOG(loglevel::info) << "Destructing Asynchronous input buffer" << std::endl; };
~AsyncDeviceInputBuffer() override {
prepareForShutdown();
FINN_LOG(loglevel::info) << "Destructing Asynchronous input buffer" << std::endl;
};

Copilot uses AI. Check for mistakes.
Comment on lines 43 to +46
driver.input(data.begin(), data.end());
std::this_thread::sleep_for(200ms);
auto results = driver.getResults();
auto results = driver.getResults(); // This should block until the results are available

Finn::vector<uint16_t> expectedResults = { 98, 50, 65476, 65493, 27 };
Finn::vector<uint16_t> expectedResults = {98, 50, 65476, 65493, 27};
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

Removing the sleep assumes getResults() will block until data is ready, but if the async pipeline hasn't produced anything yet, this may hang or fail. Add an explicit wait or callback notification to ensure data availability before calling getResults().

Copilot uses AI. Check for mistakes.
@LinusJungemann LinusJungemann merged commit 6c1f0f4 into dev Jul 17, 2025
1 check passed
LinusJungemann added a commit that referenced this pull request Jul 24, 2025
* Remove hardcoded batch size from kernel execution

* Implement setBatchSize for complete Stack

* Remove RingBuffer from Synchronous Inference and add full batch mapping

* Deduplicate batchsize in basedriver & fix unittests

* Fix integrationtests

* Change input kernel code to run concurrrently to output kernel code

* Optimize inference of lower batch sizes

* Increase packing performance

* Further optimize OpenMP

* Optimize Utils

* Some small changes

* Add example data

* Small Amounts of cleanup

* Change Driver to run without XRT managed kernels

* Add more efficient version of execute method

* Hotfix FPGA bricking

* Simplify inference interface to speed up inference

* Update unittest

* Simplify code

* Update CMake

* Fix Release Build CMakeLists

* Fix wrong old variable names in CMake

* Fix formatting

* Change format target

* Add changes to paper version

* Add final paper changes

* Add basic host mem functionality

* Add switch for Host Memory Access and fix unittests for User Managed Kernels support

* Revert timing changes for paper

* Formatting changes

* Remove unneccesary benchmark

* Small changes

* Clean up and update dependencies

* Merge dev into paperVersion

* Fix setting of Host Mem Var and update cppcheck config

* Update CI definition

* Fix typo in CI

* Remove hardcoded path from examples

* Fix linting for json files

* Expand integrationTests

* Update FPGA PCIe signatures

* Increase timelimits of jobs

* Switch CI partition to HACC for testing

* Bump Graphviz version

* Optimize CI

* Fix integrationtest path

* Update CI and add performance benchmark

* Fix paths

* Change logger and add exptected performance results to synchronous inference benchmark

* Update expected results

* Add missing path change

* Add regression tests

* Add test condition to regression test

* Fix broken bash script in CI

* Fix broken bash script in CI

* Update dependencies in CI pipeline

* Fix missing boost lib

* Fix missing libs

* Change number of processors to be correct and simplify regression tests

* Fix typo in ci

* Fix floating point comparison

* Add debug print to CI

* Add debug print to CI

* Filter colored output

* Filter colored output

* Update .gitlab-ci.yml

* Update .gitlab-ci.yml

* Update .gitlab-ci.yml

* Update remove shipped boost version and merge main to dev (#18)

* Merge dev into main for v1.2 release (#13)

* Remove hardcoded batch size from kernel execution

* Implement setBatchSize for complete Stack

* Remove RingBuffer from Synchronous Inference and add full batch mapping

* Deduplicate batchsize in basedriver & fix unittests

* Fix integrationtests

* Change input kernel code to run concurrrently to output kernel code

* Optimize inference of lower batch sizes

* Increase packing performance

* Further optimize OpenMP

* Optimize Utils

* Some small changes

* Add example data

* Small Amounts of cleanup

* Change Driver to run without XRT managed kernels

* Add more efficient version of execute method

* Hotfix FPGA bricking

* Simplify inference interface to speed up inference

* Update unittest

* Simplify code

* Update CMake

* Fix Release Build CMakeLists

* Fix wrong old variable names in CMake

* Fix formatting

* Change format target

* Add changes to paper version

* Add final paper changes

* Add basic host mem functionality

* Add switch for Host Memory Access and fix unittests for User Managed Kernels support

* Revert timing changes for paper

* Formatting changes

* Remove unneccesary benchmark

* Small changes

* Clean up and update dependencies

* Merge dev into paperVersion

* Fix setting of Host Mem Var and update cppcheck config

* Update CI definition

* Fix typo in CI

* Remove hardcoded path from examples

* Fix linting for json files

* Expand integrationTests

* Update FPGA PCIe signatures

* Increase timelimits of jobs

* Switch CI partition to HACC for testing

* Bump Graphviz version

* Optimize CI

* Fix integrationtest path

* Update CI and add performance benchmark

* Fix paths

* Change logger and add exptected performance results to synchronous inference benchmark

* Update expected results

* Add missing path change

* Add regression tests

* Add test condition to regression test

* Fix broken bash script in CI

* Fix broken bash script in CI

* Update dependencies in CI pipeline

* Fix missing boost lib

* Fix missing libs

* Change number of processors to be correct and simplify regression tests

* Fix typo in ci

* Fix floating point comparison

* Add debug print to CI

* Add debug print to CI

* Filter colored output

* Filter colored output

* Update .gitlab-ci.yml

* Update .gitlab-ci.yml

* Update .gitlab-ci.yml

* Pending changes exported from your codespace

* Remove boost form being shipped with the driver

* Update CI

* Refactor build configuration: remove mdspan submodule, update CMakeLists for output directories, and enhance FINNDriver with static configuration check

* update README.md

* Format FinnDatatypes.hpp

* Fix linting

* Update src/FINNCppDriver/FINNDriver.cpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update src/FINNCppDriver/core/DeviceBuffer/DeviceBuffer.hpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update src/FINNCppDriver/FINNDriver.cpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update readme

* Remove boost from the driver (#20)

* Remove boost logging and program options

* Add SPSCQueue

* Remove Ringbuffer

* Remove boost completely

* Update CI

* Fix linting

* Add missing includes

* Fix Integrationtests

* Fix ODR

* Fix bug where batch size is not set correctly

* Fix linting

* Update dependencies

* Bump module versions

* Update include files

* Fix Async Inference

* Add Asynchronous Inference option (#21)

* Disable FPGA resets when running unittests

* Finish async implementation

* Update CI

* Add missing header

* Fix aligned alloc

* Increase timeout

* Linting and CI timeout

* Increase CI timeout

* Fix warnings

* Add unused annotion

* Add missing doxygen documentation

* Merge branch 'main' into dev

* Fix small details

* Merge main into dev

* Update README.md

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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