From 7b9f2cd8548498580f3ff77a9290ad334771009e Mon Sep 17 00:00:00 2001 From: rdb Date: Tue, 29 Nov 2022 15:43:18 +0100 Subject: [PATCH] pstats: Support nested start/stop pairs in the server, accept 3.1 Currently, the client doesn't send nested start/stop pairs, but there is no good reason not to handle this case in the server, and in some cases it is useful to send them (I will soon make changes on master to take advantage of this). Client version 3.1 indicates that the client may send nested start/stop pairs. --- panda/src/pstatclient/pStatProperties.cxx | 7 +- pandatool/src/pstatserver/pStatReader.cxx | 3 +- pandatool/src/pstatserver/pStatView.cxx | 109 ++++++++++------------ 3 files changed, 55 insertions(+), 64 deletions(-) diff --git a/panda/src/pstatclient/pStatProperties.cxx b/panda/src/pstatclient/pStatProperties.cxx index cce31e693f..9537f50a90 100644 --- a/panda/src/pstatclient/pStatProperties.cxx +++ b/panda/src/pstatclient/pStatProperties.cxx @@ -27,9 +27,10 @@ using std::string; static const int current_pstat_major_version = 3; static const int current_pstat_minor_version = 0; -// Initialized at 2.0 on 51801, when version numbers were first added. -// Incremented to 2.1 on 52101 to add support for TCP frame data. Incremented -// to 3.0 on 42805 to bump TCP headers to 32 bits. +// Initialized at 2.0 on 5/18/01, when version numbers were first added. +// Incremented to 2.1 on 5/21/01 to add support for TCP frame data. +// Incremented to 3.0 on 4/28/05 to bump TCP headers to 32 bits. +// Incremented to 3.1 on 11/29/22 to support nested start/stop pairs. /** * Returns the current major version number of the PStats protocol. This is diff --git a/pandatool/src/pstatserver/pStatReader.cxx b/pandatool/src/pstatserver/pStatReader.cxx index 02b01f26a2..7c1fe6fca5 100644 --- a/pandatool/src/pstatserver/pStatReader.cxx +++ b/pandatool/src/pstatserver/pStatReader.cxx @@ -193,7 +193,8 @@ handle_client_control_message(const PStatClientControlMessage &message) { if (message._major_version != server_major_version || (message._major_version == server_major_version && - message._minor_version > server_minor_version)) { + message._minor_version > server_minor_version && + (message._major_version != 3 || message._minor_version != 1))) { _monitor->bad_version(message._client_hostname, message._client_progname, message._major_version, message._minor_version, server_major_version, server_minor_version); diff --git a/pandatool/src/pstatserver/pStatView.cxx b/pandatool/src/pstatserver/pStatView.cxx index 2e255273f7..092f74f276 100644 --- a/pandatool/src/pstatserver/pStatView.cxx +++ b/pandatool/src/pstatserver/pStatView.cxx @@ -26,61 +26,57 @@ */ class FrameSample { public: - void data_point(double time, bool is_start, FrameSample *started) { - _touched = true; + void start(double time, FrameSample *started) { + // Keep track of nested start/stop pairs. We only consider the outer one. + if (_started++ > 0) { + return; + } - // We only consider events that change the startstop state. With two - // consecutive 'start' events, for instance, we ignore the second one. + nassertv(!_pushed); + _net_time -= time; + push_all(time, started); + nassertv(_next == nullptr && _prev == nullptr); + _prev = started->_prev; + _next = started; + _prev->_next = this; + started->_prev = this; + } -/* - * *** That's not quite the right thing to do. We should keep track of the - * nesting level and bracket things correctly, so that we ignore the second - * start and the *first* stop, but respect the outer startstop. For the short - * term, this works, because the client is already doing this logic and won't - * send us nested startstop pairs, but we'd like to generalize this in the - * future so we can deal with these nested pairs properly. - */ - nassertv(is_start != _is_started); + void stop(double time, FrameSample *started) { + nassertv(_started > 0); + if (--_started > 0) { + return; + } - _is_started = is_start; + nassertv(_next != nullptr && _prev != nullptr); if (_pushed) { - nassertv(!_is_started); - nassertv(_next != nullptr && _prev != nullptr); _prev->_next = _next; _next->_prev = _prev; _next = _prev = nullptr; } else { - if (_is_started) { - _net_time -= time; - push_all(time, started); - nassertv(_next == nullptr && _prev == nullptr); - _prev = started->_prev; - _next = started; - _prev->_next = this; - started->_prev = this; - } else { - _net_time += time; - nassertv(_next != nullptr && _prev != nullptr); - _prev->_next = _next; - _next->_prev = _prev; - _next = _prev = nullptr; - pop_one(time, started); - } + _net_time += time; + _prev->_next = _next; + _next->_prev = _prev; + _next = _prev = nullptr; + pop_one(time, started); } } + +private: void push(double time) { if (!_pushed) { _pushed = true; - if (_is_started) { + if (_started > 0) { _net_time += time; } } } + void pop(double time) { if (_pushed) { _pushed = false; - if (_is_started) { + if (_started > 0) { _net_time -= time; } } @@ -103,14 +99,14 @@ public: } } +public: FrameSample *_next = nullptr; FrameSample *_prev = nullptr; - bool _touched = false; - bool _is_started = false; + double _net_time = 0.0; + int _started = 0; + int _count = 0; bool _pushed = false; bool _is_new = false; - int _count = 0; - double _net_time = 0.0; }; /** @@ -301,28 +297,21 @@ update_time_data(const PStatFrameData &frame_data) { if (_client_data->get_child_distance(_constraint, collector_index) >= 0) { // Here's a data point we care about: anything at constraint level or // below. - if (is_start == samples[collector_index]._is_started) { - if (!is_start) { - // A "stop" in the middle of a frame implies a "start" since time - // 0 (that is, since the first data point in the frame). - samples[collector_index].data_point(frame_data.get_time(0), true, &started); - samples[collector_index].data_point(frame_data.get_time(i), is_start, &started); - } else { - // An extra "start" for a collector that's already started is an - // error. - nout << "Unexpected data point for " - << _client_data->get_collector_fullname(collector_index) - << "\n"; - } + if (is_start) { + samples[collector_index].start(frame_data.get_time(i), &started); + samples[collector_index]._count++; } else { - samples[collector_index].data_point(frame_data.get_time(i), is_start, &started); - if (is_start) { - samples[collector_index]._count++; - } - if (!samples[collector_index]._is_new) { - samples[collector_index]._is_new = true; - ++new_collectors; + // A "stop" in the middle of a frame implies a "start" since time + // 0 (that is, since the first data point in the frame). + if (samples[collector_index]._started == 0) { + samples[collector_index].start(frame_data.get_time(0), &started); } + samples[collector_index].stop(frame_data.get_time(i), &started); + } + + if (!samples[collector_index]._is_new) { + samples[collector_index]._is_new = true; + ++new_collectors; } } } @@ -331,8 +320,8 @@ update_time_data(const PStatFrameData &frame_data) { // Make sure everything is stopped. Samples::iterator si; for (i = 0, si = samples.begin(); si != samples.end(); ++i, ++si) { - if ((*si)._is_started) { - (*si).data_point(frame_data.get_end(), false, &started); + if ((*si)._started > 0) { + (*si).stop(frame_data.get_end(), &started); } }