From 10931dceb8d773fdbabdd1c985315448f36459ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?kleines=20Filmr=C3=B6llchen?= Date: Sat, 24 Jun 2023 15:12:35 +0200 Subject: [PATCH] SystemMonitor: Only read process command line once Process command line reading took up 50% (!) of all of SystemMonitor's work. However, a process's command line per definition never changes, so we can read it once and carry it over. Also, if we couldn't read a process's command line once, it is close to impossible that we'll ever be able to read it in the future. Therefore, skip reading such command lines again as well. This commit also converts the command line itself to use String, while we're at it. --- .../SystemMonitor/ProcessModel.cpp | 27 ++++++---------- .../Applications/SystemMonitor/ProcessModel.h | 31 +++++++++++++++++-- 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/Userland/Applications/SystemMonitor/ProcessModel.cpp b/Userland/Applications/SystemMonitor/ProcessModel.cpp index 9b6e45b9cc6..3498a6be688 100644 --- a/Userland/Applications/SystemMonitor/ProcessModel.cpp +++ b/Userland/Applications/SystemMonitor/ProcessModel.cpp @@ -226,7 +226,7 @@ GUI::Variant ProcessModel::data(GUI::ModelIndex const& index, GUI::ModelRole rol case Column::Name: return thread.current_state.name; case Column::Command: - return thread.current_state.command; + return thread.current_state.command.visit([](String const& cmdline) { return cmdline; }, [](auto const&) { return ""_short_string; }); case Column::Syscalls: return thread.current_state.syscall_count; case Column::InodeFaults: @@ -296,7 +296,7 @@ GUI::Variant ProcessModel::data(GUI::ModelIndex const& index, GUI::ModelRole rol return DeprecatedString::formatted("{} (*)", thread.current_state.name); return thread.current_state.name; case Column::Command: - return thread.current_state.command; + return thread.current_state.command.visit([](String const& cmdline) { return cmdline; }, [](auto const&) { return ""_short_string; }); case Column::Syscalls: return thread.current_state.syscall_count; case Column::InodeFaults: @@ -422,25 +422,16 @@ Vector ProcessModel::matches(StringView searching, unsigned fla return found_indices; } -static ErrorOr try_read_command_line(pid_t pid) +ErrorOr ProcessModel::read_command_line(pid_t pid) { - auto file = TRY(Core::File::open(DeprecatedString::formatted("/proc/{}/cmdline", pid), Core::File::OpenMode::Read)); + auto file = TRY(Core::File::open(TRY(String::formatted("/proc/{}/cmdline", pid)), Core::File::OpenMode::Read)); auto data = TRY(file->read_until_eof()); auto json = TRY(JsonValue::from_string(StringView { data.bytes() })); auto array = json.as_array().values(); - return DeprecatedString::join(" "sv, array); + return String::join(" "sv, array); } -static DeprecatedString read_command_line(pid_t pid) -{ - auto string_or_error = try_read_command_line(pid); - if (string_or_error.is_error()) { - return ""; - } - return string_or_error.release_value(); -} - -ErrorOr ProcessModel::initialize_process_statistics_file() +ErrorOr ProcessModel::ensure_process_statistics_file() { if (!m_process_statistics_file || !m_process_statistics_file->is_open()) m_process_statistics_file = TRY(Core::File::open("/sys/kernel/processes"sv, Core::File::OpenMode::Read)); @@ -450,7 +441,7 @@ ErrorOr ProcessModel::initialize_process_statistics_file() void ProcessModel::update() { - auto result = initialize_process_statistics_file(); + auto result = ensure_process_statistics_file(); if (result.is_error()) { dbgln("Process model couldn't be updated: {}", result.release_error()); return; @@ -489,6 +480,8 @@ void ProcessModel::update() auto thread_data = m_threads.ensure(tid, [&] { return make_ref_counted(process_state); }); thread_data->previous_state = move(thread_data->current_state); thread_data->current_state = move(state); + thread_data->read_command_line_if_necessary(); + if (auto maybe_thread_index = process_state.threads.find_first_index(thread_data); maybe_thread_index.has_value()) { process_state.threads[maybe_thread_index.value()] = thread_data; } else { @@ -511,7 +504,6 @@ void ProcessModel::update() state.kernel = process.kernel; state.executable = process.executable; state.name = thread.name; - state.command = read_command_line(process.pid); state.uid = process.uid; state.state = thread.state; state.user = process.username; @@ -554,7 +546,6 @@ void ProcessModel::update() state.kernel = process.kernel; state.executable = process.executable; state.name = process.name; - state.command = read_command_line(process.pid); state.uid = process.uid; state.state = "Zombie"; state.user = process.username; diff --git a/Userland/Applications/SystemMonitor/ProcessModel.h b/Userland/Applications/SystemMonitor/ProcessModel.h index 6909196d961..9f6a9f9d7f1 100644 --- a/Userland/Applications/SystemMonitor/ProcessModel.h +++ b/Userland/Applications/SystemMonitor/ProcessModel.h @@ -55,6 +55,8 @@ public: __Count }; + static ErrorOr read_command_line(pid_t pid); + static ProcessModel& the(); static NonnullRefPtr create() { return adopt_ref(*new ProcessModel); } @@ -94,6 +96,11 @@ private: struct Process; + enum class EmptyCommand : u8 { + NotInitialized, + PermissionError, + }; + struct ThreadState { pid_t tid { 0 }; pid_t pid { 0 }; @@ -105,7 +112,7 @@ private: bool kernel { false }; DeprecatedString executable { "" }; DeprecatedString name { "" }; - DeprecatedString command { "" }; + Variant command { EmptyCommand::NotInitialized }; uid_t uid { 0 }; DeprecatedString state { "" }; DeprecatedString user { "" }; @@ -202,6 +209,26 @@ private: { return current_state.tid == current_state.process.pid; } + + void read_command_line_if_necessary() + { + // Most likely the previous state still has a command line which we can copy over. + // Or, reading the command line was not allowed, e.g. on a Kernel process. + if ((previous_state.command.has() && !current_state.command.has()) + || (previous_state.command.has() && previous_state.command.get() == EmptyCommand::PermissionError)) { + current_state.command = previous_state.command; + return; + } + + auto maybe_command_line = read_command_line(current_state.pid); + if (!maybe_command_line.is_error()) { + current_state.command = maybe_command_line.value(); + previous_state.command = maybe_command_line.release_value(); + } else if (maybe_command_line.error().code() == EPERM || maybe_command_line.error().code() == EACCES) { + current_state.command = EmptyCommand::PermissionError; + previous_state.command = EmptyCommand::PermissionError; + } + } }; struct Process { @@ -240,7 +267,7 @@ private: int thread_model_row(Thread const& thread) const; - ErrorOr initialize_process_statistics_file(); + ErrorOr ensure_process_statistics_file(); OwnPtr m_process_statistics_file;