minor performance optimization implementations by ThePyLord · Pull Request #2 · ThePyLord/hft_sim

ThePyLord · 2026-01-04T07:37:12Z

Attempted to make some performance optimizations to order matching and add APIs for future planned optimizations.

Copilot

Pull request overview

This PR attempts to implement performance optimizations for order matching in a high-frequency trading simulator, along with introducing new infrastructure for future optimizations. However, the changes introduce several critical bugs that undermine both correctness and performance goals.

Key changes:

Removed mutex synchronization from Logger to reduce locking overhead (introduces critical race conditions)
Simplified ReorderingBuffer API by removing stream_id parameter
Added hardware-accelerated CRC32C implementation for ARM processors
Introduced MemoryPool template class for object pooling
Refactored order matching logic with additional debug logging
Updated test infrastructure and added new test files

Reviewed changes

Copilot reviewed 12 out of 15 changed files in this pull request and generated 23 comments.

Show a summary per file

File	Description
util/Logger.cpp	Removed mutex locks from all logging methods, creating race conditions
tests/test_reordering_buffer.cpp	Updated tests to use simplified API without stream_id
tests/test_lockfree_queue.cpp	Added benchmark test, but implementation has critical flaws
tests/test_crc32c.cpp	New test file for CRC32C validation with known test vectors
tests/test_memory_pool.cpp	New test file for memory pool, but contains unused allocation
src/net/ReorderingBuffer.cpp	Simplified API by removing stream_id parameter from packets
src/net/Crc32c.cpp	Added ARM CRC hardware acceleration support
src/OrderBook.cpp	Refactored matching logic with extensive debug logging added
main.cpp	Updated threading model to single consumer, changed indentation to tabs
include/net/ReorderingBuffer.h	Updated header to reflect simplified API with data_t typedef
include/net/Crc32c.h	New header file for CRC32C functions
include/MemoryPool.h	New memory pool template class with free list implementation
README.md	Added documentation for features and getting started guide
CMakeLists.txt	Added ARM CRC compiler flags and test infrastructure updates
.gitignore	Added .cache/ directory to ignored files

Comments suppressed due to low confidence (1)

util/Logger.cpp:84

Removing the mutex lock from logImpl creates a race condition when multiple threads attempt to log concurrently. This can result in interleaved output to console and file streams, corrupted log entries, and potential crashes when file operations are performed simultaneously from different threads. The timestamp formatting using localtime() is also not thread-safe.

void Logger::logImpl(LogLevel level, const std::string& msg) {
    // Timestamp
    auto now = std::chrono::system_clock::now();
    auto in_time_t = std::chrono::system_clock::to_time_t(now);
    std::stringstream ss;
    ss << std::put_time(std::localtime(&in_time_t), "%Y-%m-%d %H:%M:%S");
    // Log format: [LEVEL][timestamp] message
    std::string log_line = "[" + levelToString(level) + "][" + ss.str() + "] " + msg + "\n";
    if (console_enabled_) {
        std::cout << log_line;
    }
    if (file_enabled_ && file_stream_.is_open()) {
        file_stream_ << log_line;
        file_stream_.flush();

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-04T07:42:44Z

 #include <atomic>
 #include <chrono>
 #include <iostream>
+#include <format>


The #include <format> directive at line 7 is incompatible with the locale functionality being used. The std::format function with the ":L" specifier requires C++20 format support which may not be available on all platforms, and mixing it with locale-specific formatting can cause compilation errors or runtime issues.

Suggested change

#include <format>

Copilot · 2026-01-04T07:42:48Z

 #include "net/Crc32c.h"

 #include <cstddef>
+#include <zlib.h>


The header file is included but never used in this implementation. The zlib.h include at line 4 appears unnecessary as there are no zlib functions called in the code. This should be removed to avoid unnecessary dependencies.

Suggested change

#include <zlib.h>

Copilot · 2026-01-04T07:42:48Z

+// Main CRC32C function (Incomplete)
 uint32_t crc32c(const uint8_t* data, size_t len) noexcept {
    // TODO: Add runtime CPU feature detection and use hardware when available
    return crc32c_sw(data, len);


The comment on line 107 is incorrect - this is not actually a complete implementation as stated. The comment at line 107 says "Main CRC32C function (Incomplete)" but the actual implementation always uses software fallback and doesn't implement runtime CPU detection as mentioned in the TODO on line 109, making the "incomplete" label accurate.

Copilot · 2026-01-04T07:42:48Z

+      // Handle market orders immediately
+
+        logger.debug(std::format("Adding market order {} to order book", getSideName(order.getSide())));
+        logger.debug(std::format("Order price: {}", order.getPrice().value()));


The market order check at line 24 attempts to access the price value of a market order, but market orders typically don't have prices. This will either cause an exception if getPrice() returns an empty optional, or log incorrect information if a price happens to be set on a market order.

Suggested change

logger.debug(std::format("Order price: {}", order.getPrice().value()));

if (order.getPrice().has_value()) {

logger.debug(std::format("Order price: {}", order.getPrice().value()));

} else {

logger.debug("Order price: MARKET");

}

Copilot · 2026-01-04T07:42:49Z

+    for (auto& producer : producers) {
+        if (producer.joinable()) {
+            producer.join();
+        }
+    }
+
+    producers_done.store(true, std::memory_order_release);
+
+    std::thread consumer([&]() {
        for (int i = 0; i < N; ++i) {
            auto val = q.pop();
            while (!val) std::this_thread::yield();
        }


The consumer thread is started after all producers have finished (line 106), which defeats the purpose of testing concurrent producer-consumer behavior. The benchmark should start the consumer before or concurrently with the producers to accurately measure the performance of concurrent operations.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

🟡 Changes recommended

There are confirmed build-breaking issues (API change not updated in all call sites, CMake gtest_discover_tests usage) and correctness bugs in OrderBook matching logic.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (6)

util/Logger.cpp:75

logImpl() no longer takes the logger mutex. This makes writes to file_stream_/console_enabled_/file_enabled_ racy, and std::localtime() is not thread-safe without external synchronization.

void Logger::logImpl(LogLevel level, const std::string& msg) {
    // Timestamp
    auto now = std::chrono::system_clock::now();
    auto in_time_t = std::chrono::system_clock::to_time_t(now);
    std::stringstream ss;

src/OrderBook.cpp:180

match_orders() erases price levels unconditionally: if (bidIter != bids.end()) / if (askIter != asks.end()) will almost always be true, so the best bid/ask levels get removed even when non-empty and matching never happens.

    while (!bids.empty() && !asks.empty()) {
        // If the bid or ask is empty, remove it from the book
        // Safely handle iterator invalidation
        if (bidIter != bids.end()) {
            bidIter = bids.erase(bidIter);

src/net/Crc32c.cpp:6

Crc32c.cpp adds a hard include on <zlib.h> (and ) but neither header is used in this file. This introduces a new system dependency that can break builds on systems without zlib headers installed.

#include <cstddef>
#include <zlib.h>
#include <cstdint>
#include <iostream>

CMakeLists.txt:114

gtest_discover_tests() is being called with an extra PRIVATE argument (not part of the CMake API) and the test is also registered via add_test(), which can lead to configuration errors or duplicate test registration.

    target_link_libraries(${TEST_NAME} PRIVATE gtest gtest_main hsnet)
    gtest_discover_tests(${TEST_NAME} PRIVATE)
    add_test(NAME ${TEST_NAME} COMMAND ${TEST_OUTPUT_DIR}/${TEST_NAME})
    message("Writing test executable: ${TEST_NAME} with source: ${TEST_SOURCE} to ${TEST_OUTPUT_DIR}")

tests/test_lockfree_queue.cpp:94

This benchmark test sets a global locale (which can throw if the locale isn't installed) and runs with N=100'000'000, which is likely to make the normal test suite extremely slow or time out in CI. Consider disabling it by default (DISABLED_) and using a smaller N for occasional local runs.

TEST(LockFreeQueueBenchmark, ProducerConsumer) {
    std::locale::global(std::locale("en_US.UTF-8"));
    LockFreeQueue<int> q;
    const int N = 100'000'000;
    std::atomic<bool> producers_done{false};

util/Logger.cpp:45

enableFile() opens/closes file_stream_ without synchronization. Without the mutex this can race with logImpl() writing/flushing the stream, which is undefined behavior.

void Logger::enableFile(bool enable) {
    file_enabled_ = enable;
    if (file_enabled_ && !file_stream_.is_open()) {
        file_stream_.open(log_file_, std::ios::app);
    } else if (!file_enabled_ && file_stream_.is_open()) {
        file_stream_.close();

Files reviewed: 12/15 changed files
Comments generated: 5
Review effort level: Lite

+    bool add(uint64_t sequence, data_t data);
+
    // Get next in-sequence packet, returns empty if none available
-    std::optional<std::pair<std::vector<uint8_t>, uint32_t>> get_next();
-    
+    std::optional<data_t> get_next();
+


 #include <algorithm>
 #include <format>
 #include <cassert>
-#include "Logger.h"
+// #include "Logger.h"



+    if (bids.empty() || asks.empty()) {
+        logger.debug("Other side of book is empty, cannot match market order");
+        return;
+    }


+#include <iostream>
+#include <cstddef>
+#include <cstdint>
+#include <memory>
+


 void Logger::setLogFile(const std::string& filename) {
-    std::lock_guard<std::mutex> lock(mutex_);
    log_file_ = filename;
+    std::cout << "Log file set to: " << log_file_ << std::endl;
    if (file_stream_.is_open()) {
        file_stream_.close();


ThePyLord added 6 commits November 8, 2025 13:56

Fixed order matching and changed to MPSC architecture.

8c3d57d

Updated .gitignore and CMakeLists

9139454

Implemented basic memory pool for future use.

1c7099e

Implemented CRC32 hardware option.

8fe2fbe

Updated README

8d075c4

ReorderingBuffer refactor, simplified the API.

d53560e

ThePyLord self-assigned this Jan 4, 2026

ThePyLord requested a review from Copilot January 4, 2026 07:37

ThePyLord added documentation Improvements or additions to documentation enhancement New feature or request labels Jan 4, 2026

Copilot started reviewing on behalf of ThePyLord January 4, 2026 07:37 View session

Copilot AI reviewed Jan 4, 2026

View reviewed changes

Copilot AI reviewed Jan 4, 2026

ThePyLord requested a lite review from Copilot September 4, 2026 12:54

Copilot started reviewing on behalf of ThePyLord September 4, 2026 12:54 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Navigation Menu

minor performance optimization implementations - #2

minor performance optimization implementations#2
ThePyLord wants to merge 6 commits into
mainfrom
ds_optimization

ThePyLord commented Jan 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 4, 2026

Uh oh!

Uh oh!

Copilot AI Jan 4, 2026

Uh oh!

Copilot AI Jan 4, 2026

Uh oh!

Copilot AI Jan 4, 2026

Uh oh!

Copilot AI Jan 4, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-        logger.debug(std::format("Order price: {}", order.getPrice().value()));
+        if (order.getPrice().has_value()) {
+            logger.debug(std::format("Order price: {}", order.getPrice().value()));
+        } else {
+            logger.debug("Order price: MARKET");
+        }

Conversation

ThePyLord commented Jan 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (6)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants