clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed.
================ Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:35 + + if (m_event_type == progressStart) + m_message = message; ---------------- ================ Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:102 + : m_start_event(start_event), + m_start_event_timestamp( + std::chrono::system_clock::now().time_since_epoch()), ---------------- remove m_start_event_timestamp as it is no longer needed since ProgressEvent has a timestamp now. ================ Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:106 + +bool ProgressEventManager::TryReport() { + // We only report if we have already started reporting or if enough time has ---------------- rename to "ReportIfNeeded()"? ================ Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:109-118 + if (!m_start_event.Reported() && + std::chrono::system_clock::now().time_since_epoch() - + m_start_event.GetTimestamp() < + std::chrono::seconds(1)) + return false; + + if (!m_start_event.Reported() && !Finished()) { ---------------- ================ Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:116-117 + if (!m_start_event.Reported() && !Finished()) { + m_report_callback(m_start_event); + m_start_event.MarkAsReported(); + } ---------------- We have many places in the code that are reporting progress and then having to manually mark the event as reported. We should make a ProgressEvent::Report() function that looks something like: ``` void ProgressEvent::Report(ProgressEventReportCallback report_callback) { if (!m_reported) { m_reported = true; report_callback(*this); } } ``` ================ Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:120-121 + if (m_last_update_event && !m_last_update_event->Reported()) { + m_report_callback(*m_last_update_event); + m_last_update_event->MarkAsReported(); + } ---------------- ================ Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:146 + while (!m_aux_reporter_thread_should_exit) { + std::this_thread::sleep_for(std::chrono::seconds(2)); + ReportWaitingEvents(); ---------------- We have a 1 second timeout before events should be reported, so we should probably sleep a shorter amount of time than 2 seconds as if the events come in such that a progress start event is queued right after this loop sleeps for 2 seconds, it can now be three seconds before an event is reported. How about 250 ms? That way the most we will wait before reporting a start event is 1.25 seconds. Also since we expect the thread to exit in ~ProgressEventFilterQueue, we don't want to have to wait 2 seconds during shut down. ================ Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:157 + +void ProgressEventFilterQueue::ReportWaitingEvents() { + std::lock_guard<std::mutex> locker(m_reporter_mutex); ---------------- Should this be named "ReportStartEvents()"? Is that the only thing this is doing here? ================ Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:165 + else if (event_manager->TryReport()) + m_sorted_event_managers + .pop(); // we remove it from the queue as it started reporting ---------------- If we are using m_sorted_event_managers only for reporting start events, we should rename to "m_unreported_start_events" ================ Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:170 + else + break; // If we couldn't report it, then the next event in the queue won't + // be able as well, as it came later. ---------------- Ok so this class is a start event reporter then? ================ Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:132 + /// Thread used to invoke \a ReportWaitingEvents periodically. + std::thread m_aux_reporter_thread; + bool m_aux_reporter_thread_should_exit; ---------------- m_thread should suffice here. ================ Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:133 + std::thread m_aux_reporter_thread; + bool m_aux_reporter_thread_should_exit; + /// Mutex that prevents running \a Push and \a ReportWaitingEvents ---------------- m_thread_should_exit should suffice. ================ Comment at: lldb/tools/lldb-vscode/ProgressEvent.h:136 + /// simultaneously, as both read and modify the same underlying objects. + std::mutex m_reporter_mutex; }; ---------------- m_mutex is fine unless we get more than one mutex in this class. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101128/new/ https://reviews.llvm.org/D101128 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits