From 1e90a3a542cecf6510e2d615012f96e2fc10bd27 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Wed, 11 Aug 2021 20:41:29 +0200 Subject: [PATCH] Kernel: Make sys$perf_register_string() generate the string ID's Making userspace provide a global string ID was silly, and made the API extremely difficult to use correctly in a global profiling context. Instead, simply make the kernel do the string ID allocation for us. This also allows us to convert the string storage to a Vector in the kernel (and an array in the JSON profile data.) --- Kernel/PerformanceEventBuffer.cpp | 15 ++++++++------- Kernel/PerformanceEventBuffer.h | 4 ++-- Kernel/Process.h | 2 +- Kernel/Syscalls/perf_event.cpp | 4 ++-- Userland/DevTools/Profiler/Profile.cpp | 13 ++++++------- Userland/Libraries/LibC/serenity.cpp | 4 ++-- Userland/Libraries/LibC/serenity.h | 2 +- Userland/Libraries/LibJS/Heap/Heap.cpp | 4 ++-- 8 files changed, 24 insertions(+), 24 deletions(-) diff --git a/Kernel/PerformanceEventBuffer.cpp b/Kernel/PerformanceEventBuffer.cpp index db7aa3fc20..ba05b1a228 100644 --- a/Kernel/PerformanceEventBuffer.cpp +++ b/Kernel/PerformanceEventBuffer.cpp @@ -167,9 +167,9 @@ template bool PerformanceEventBuffer::to_json_impl(Serializer& object) const { { - auto strings = object.add_object("strings"); + auto strings = object.add_array("strings"); for (auto& it : m_strings) { - strings.add(String::number(it.key), it.value->view()); + strings.add(it.view()); } } @@ -305,13 +305,14 @@ void PerformanceEventBuffer::add_process(const Process& process, ProcessEventTyp } } -KResult PerformanceEventBuffer::register_string(FlatPtr string_id, NonnullOwnPtr string) +KResultOr PerformanceEventBuffer::register_string(NonnullOwnPtr string) { - m_strings.set(string_id, move(string)); + FlatPtr string_id = m_strings.size(); - // FIXME: Switch m_strings to something that can signal allocation failure, - // and then propagate such failures here. - return KSuccess; + if (!m_strings.try_append(move(string))) + return ENOBUFS; + + return string_id; } } diff --git a/Kernel/PerformanceEventBuffer.h b/Kernel/PerformanceEventBuffer.h index 6278ad6ef4..897f2ff717 100644 --- a/Kernel/PerformanceEventBuffer.h +++ b/Kernel/PerformanceEventBuffer.h @@ -120,7 +120,7 @@ public: void add_process(const Process&, ProcessEventType event_type); - KResult register_string(FlatPtr string_id, NonnullOwnPtr); + KResultOr register_string(NonnullOwnPtr); private: explicit PerformanceEventBuffer(NonnullOwnPtr); @@ -133,7 +133,7 @@ private: size_t m_count { 0 }; NonnullOwnPtr m_buffer; - HashMap> m_strings; + NonnullOwnPtrVector m_strings; }; extern bool g_profiling_all_threads; diff --git a/Kernel/Process.h b/Kernel/Process.h index 99711ed655..26dcc49d11 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -400,7 +400,7 @@ public: KResultOr sys$pledge(Userspace); KResultOr sys$unveil(Userspace); KResultOr sys$perf_event(int type, FlatPtr arg1, FlatPtr arg2); - KResultOr sys$perf_register_string(FlatPtr string_id, Userspace, size_t); + KResultOr sys$perf_register_string(Userspace, size_t); KResultOr sys$get_stack_bounds(Userspace stack_base, Userspace stack_size); KResultOr sys$ptrace(Userspace); KResultOr sys$sendfd(int sockfd, int fd); diff --git a/Kernel/Syscalls/perf_event.cpp b/Kernel/Syscalls/perf_event.cpp index 99cffb27b1..09b9db6ee0 100644 --- a/Kernel/Syscalls/perf_event.cpp +++ b/Kernel/Syscalls/perf_event.cpp @@ -21,7 +21,7 @@ KResultOr Process::sys$perf_event(int type, FlatPtr arg1, FlatPtr arg2) return events_buffer->append(type, arg1, arg2, nullptr); } -KResultOr Process::sys$perf_register_string(FlatPtr string_id, Userspace user_string, size_t user_string_length) +KResultOr Process::sys$perf_register_string(Userspace user_string, size_t user_string_length) { VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this) auto* events_buffer = current_perf_events_buffer(); @@ -32,7 +32,7 @@ KResultOr Process::sys$perf_register_string(FlatPtr string_id, Userspac if (string_or_error.is_error()) return string_or_error.error(); - return events_buffer->register_string(string_id, string_or_error.release_value()); + return events_buffer->register_string(string_or_error.release_value()); } } diff --git a/Userland/DevTools/Profiler/Profile.cpp b/Userland/DevTools/Profiler/Profile.cpp index 61f0f0524a..6597022239 100644 --- a/Userland/DevTools/Profiler/Profile.cpp +++ b/Userland/DevTools/Profiler/Profile.cpp @@ -209,15 +209,14 @@ Result, String> Profile::load_from_perfcore_file(const St kernel_elf = make(file_or_error.value()->bytes()); auto strings_value = object.get_ptr("strings"sv); - if (!strings_value || !strings_value->is_object()) - return String { "Malformed profile (strings is not an object)" }; + if (!strings_value || !strings_value->is_array()) + return String { "Malformed profile (strings is not an array)" }; HashMap profile_strings; - strings_value->as_object().for_each_member([&](String const& key, JsonValue const& value) { - auto string_id = key.to_uint(); - VERIFY(string_id.has_value()); - profile_strings.set(string_id.value(), value.to_string()); - }); + for (FlatPtr string_id = 0; string_id < strings_value->as_array().size(); ++string_id) { + auto& value = strings_value->as_array().at(string_id); + profile_strings.set(string_id, value.to_string()); + } auto events_value = object.get_ptr("events"); if (!events_value || !events_value->is_array()) diff --git a/Userland/Libraries/LibC/serenity.cpp b/Userland/Libraries/LibC/serenity.cpp index cd99ba64ee..9120222fa9 100644 --- a/Userland/Libraries/LibC/serenity.cpp +++ b/Userland/Libraries/LibC/serenity.cpp @@ -95,9 +95,9 @@ int perf_event(int type, uintptr_t arg1, FlatPtr arg2) __RETURN_WITH_ERRNO(rc, rc, -1); } -int perf_register_string(uintptr_t string_id, char const* string, size_t string_length) +int perf_register_string(char const* string, size_t string_length) { - int rc = syscall(SC_perf_register_string, string_id, string, string_length); + int rc = syscall(SC_perf_register_string, string, string_length); __RETURN_WITH_ERRNO(rc, rc, -1); } diff --git a/Userland/Libraries/LibC/serenity.h b/Userland/Libraries/LibC/serenity.h index 6d9ec2fc94..3919c911d2 100644 --- a/Userland/Libraries/LibC/serenity.h +++ b/Userland/Libraries/LibC/serenity.h @@ -119,7 +119,7 @@ enum { #define PERF_EVENT_MASK_ALL (~0ull) int perf_event(int type, uintptr_t arg1, uintptr_t arg2); -int perf_register_string(uintptr_t string_id, char const* string, size_t string_length); +int perf_register_string(char const* string, size_t string_length); int get_stack_bounds(uintptr_t* user_stack_base, size_t* user_stack_size); diff --git a/Userland/Libraries/LibJS/Heap/Heap.cpp b/Userland/Libraries/LibJS/Heap/Heap.cpp index aee2ea837f..3329489e0e 100644 --- a/Userland/Libraries/LibJS/Heap/Heap.cpp +++ b/Userland/Libraries/LibJS/Heap/Heap.cpp @@ -26,7 +26,7 @@ namespace JS { #ifdef __serenity__ -static constexpr FlatPtr gc_perf_string_id = 0x13378086; +static int gc_perf_string_id; #endif Heap::Heap(VM& vm) @@ -34,7 +34,7 @@ Heap::Heap(VM& vm) { #ifdef __serenity__ auto gc_signpost_string = "Garbage collection"sv; - perf_register_string(gc_perf_string_id, gc_signpost_string.characters_without_null_termination(), gc_signpost_string.length()); + gc_perf_string_id = perf_register_string(gc_signpost_string.characters_without_null_termination(), gc_signpost_string.length()); #endif if constexpr (HeapBlock::min_possible_cell_size <= 16) {