minor performance optimization implementations #2
Conversation
ThePyLord
added
documentation
There was a problem hiding this comment.
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.
| #include <atomic> | ||
| #include <chrono> | ||
| #include <iostream> | ||
| #include <format> |
There was a problem hiding this comment.
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.
| #include <format> |
| #include "net/Crc32c.h" | ||
|
|
||
| #include <cstddef> | ||
| #include <zlib.h> |
There was a problem hiding this comment.
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.
| #include <zlib.h> |
| // 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); |
There was a problem hiding this comment.
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.
| // 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())); |
There was a problem hiding this comment.
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.
| 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"); | |
| } |
| 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(); | ||
| } |
There was a problem hiding this comment.
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.
Attempted to make some performance optimizations to order matching and add APIs for future planned optimizations.