Skip to content

Commit

Permalink
refactor(debug): prefer Vector over std::vector
Browse files Browse the repository at this point in the history 
Reduce our use of std::vector to reduce binary bloat:
#689

Because Debug_Server now uses Vector internally, make the vector
instrumentation tests for Debug_Server tolerant of extra vector
profile data.
  • Loading branch information
@strager
strager committed Nov 7, 2023
1 parent 1a53022 commit 402e689
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 41 deletions.
16 changes: 12 additions & 4 deletions src/quick-lint-js/debug/debug-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
#include <quick-lint-js/assert.h>
#include <quick-lint-js/container/async-byte-queue.h>
#include <quick-lint-js/container/byte-buffer.h>
#include <quick-lint-js/container/monotonic-allocator.h>
#include <quick-lint-js/container/vector-profiler.h>
#include <quick-lint-js/container/vector.h>
#include <quick-lint-js/debug/debug-server-fs.h>
#include <quick-lint-js/debug/debug-server.h>
#include <quick-lint-js/debug/find-debug-server.h>
Expand Down Expand Up @@ -108,11 +110,13 @@ std::shared_ptr<Debug_Server> Debug_Server::create() {
return instance;
}

std::vector<std::shared_ptr<Debug_Server>> Debug_Server::instances() {
Raw_Vector<std::shared_ptr<Debug_Server>> Debug_Server::instances() {
return Instance_Tracker<Debug_Server>::instances();
}

Debug_Server::Debug_Server(Create_Tag) {}
Debug_Server::Debug_Server(Create_Tag)
: tracer_backends_("Debug_Server::tracer_backends_",
new_delete_resource()) {}

Debug_Server::~Debug_Server() {
if (this->server_thread_.joinable()) {
Expand Down Expand Up @@ -399,10 +403,14 @@ void Debug_Server::publish_lsp_documents_if_needed() {
return;
}

std::vector<Trace_LSP_Document_State> document_states;
Monotonic_Allocator temporary_allocator(
"Debug_Server::publish_lsp_documents_if_needed");
Vector<Trace_LSP_Document_State> document_states("document_states",
&temporary_allocator);
{
Lock_Ptr<LSP_Documents> documents = documents_raw->lock();
document_states.reserve(documents->documents.size());
document_states.reserve(
narrow_cast<Vector_Size>(documents->documents.size()));
for (auto &[uri, doc] : documents->documents) {
document_states.push_back(Trace_LSP_Document_State{
.type = doc->trace_type(),
Expand Down
6 changes: 3 additions & 3 deletions src/quick-lint-js/debug/debug-server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <mongoose.h>
#include <quick-lint-js/container/result.h>
#include <quick-lint-js/container/vector-profiler.h>
#include <quick-lint-js/container/vector.h>
#include <quick-lint-js/port/thread.h>
#include <quick-lint-js/util/synchronized.h>
#include <string>
Expand All @@ -34,7 +35,7 @@ class Debug_Server {

public:
static std::shared_ptr<Debug_Server> create();
static std::vector<std::shared_ptr<Debug_Server>> instances();
static Raw_Vector<std::shared_ptr<Debug_Server>> instances();

explicit Debug_Server(Create_Tag);

Expand Down Expand Up @@ -109,8 +110,7 @@ class Debug_Server {

// Used by server thread only:
// Each backend is associated with one WebSocket connection.
std::vector<std::unique_ptr<Trace_Flusher_WebSocket_Backend>>
tracer_backends_;
Vector<std::unique_ptr<Trace_Flusher_WebSocket_Backend>> tracer_backends_;
#if QLJS_FEATURE_VECTOR_PROFILING
Vector_Max_Size_Histogram_By_Owner max_size_histogram_;
#endif
Expand Down
21 changes: 13 additions & 8 deletions src/quick-lint-js/util/instance-tracker.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
#pragma once

#include <memory>
#include <quick-lint-js/container/vector.h>
#include <quick-lint-js/port/vector-erase.h>
#include <quick-lint-js/util/synchronized.h>
#include <vector>

namespace quick_lint_js {
// Maintains a global list of instances of Tracked. Each Tracked must be managed
Expand All @@ -18,16 +18,16 @@ template <class Tracked>
class Instance_Tracker {
public:
static void track(std::shared_ptr<Tracked> instance) {
Lock_Ptr<std::vector<std::weak_ptr<Tracked>>> weak_instances =
Lock_Ptr<Raw_Vector<std::weak_ptr<Tracked>>> weak_instances =
weak_instances_.lock();
sanitize_instances(weak_instances);
weak_instances->push_back(std::move(instance));
}

static std::vector<std::shared_ptr<Tracked>> instances() {
std::vector<std::shared_ptr<Tracked>> instances;
static Raw_Vector<std::shared_ptr<Tracked>> instances() {
Raw_Vector<std::shared_ptr<Tracked>> instances(new_delete_resource());
{
Lock_Ptr<std::vector<std::weak_ptr<Tracked>>> weak_instances =
Lock_Ptr<Raw_Vector<std::weak_ptr<Tracked>>> weak_instances =
weak_instances_.lock();
sanitize_instances(weak_instances);
instances.reserve(weak_instances->size());
Expand All @@ -38,12 +38,14 @@ class Instance_Tracker {
}
}
}
// NOTE(strager): We cannot wink (e.g. use Linked_Bump_Allocator and return
// a Span) because std::shared_ptr destructors need to be called.
return instances;
}

private:
static void sanitize_instances(
Lock_Ptr<std::vector<std::weak_ptr<Tracked>>>& weak_instances) {
Lock_Ptr<Raw_Vector<std::weak_ptr<Tracked>>>& weak_instances) {
erase_if(*weak_instances, [](const std::weak_ptr<Tracked>& weak_instance) {
return weak_instance.expired();
});
Expand All @@ -53,8 +55,11 @@ class Instance_Tracker {
sanitize_instances(weak_instances_.lock());
}

static inline Synchronized<std::vector<std::weak_ptr<Tracked>>>
weak_instances_;
// NOTE(strager): We use Raw_Vector here instead of Vector. Otherwise, in
// vector instrumented builds, weak_instances_ might be initialized before the
// vector profiler is initialized, causing use-before-init issues.
static inline Synchronized<Raw_Vector<std::weak_ptr<Tracked>>>
weak_instances_{new_delete_resource()};
};
}

Expand Down
4 changes: 4 additions & 0 deletions src/quick-lint-js/util/synchronized.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ class Lock_Ptr;
template <class Data>
class Synchronized {
public:
// Construct Data, forwarding arguments to Data's constructor.
template <class... Args>
explicit Synchronized(Args&&... args) : data_(std::forward<Args>(args)...) {}

// Acquire the mutex. When the returned lock_ptr is destructed, the mutex is
// released.
Lock_Ptr<Data> lock();
Expand Down
57 changes: 31 additions & 26 deletions test/test-debug-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,15 +179,19 @@ TEST_F(Test_Debug_Server,
// This should eventually be called.
Span<const Trace_Vector_Max_Size_Histogram_By_Owner_Entry> entries =
event.vector_max_size_histogram_by_owner_event.entries;
EXPECT_EQ(entries.size(), 1);
if (entries.size() > 0) {
EXPECT_EQ(entries[0].owner, u8"debug server test vector"_sv);
EXPECT_THAT(entries[0].max_size_entries,
::testing::ElementsAreArray({
Trace_Vector_Max_Size_Histogram_Entry{.max_size = 0,
.count = 1},
}));
bool found_entry = false;
for (const Trace_Vector_Max_Size_Histogram_By_Owner_Entry &entry :
entries) {
if (entry.owner == u8"debug server test vector"_sv) {
found_entry = true;
EXPECT_THAT(entry.max_size_entries,
::testing::ElementsAreArray({
Trace_Vector_Max_Size_Histogram_Entry{.max_size = 0,
.count = 1},
}));
}
}
EXPECT_TRUE(found_entry);

received_vector_max_size_histogram_by_owner_event = true;
return false;
Expand Down Expand Up @@ -220,7 +224,7 @@ TEST_F(Test_Debug_Server,
TEST_F(Test_Debug_Server, vector_profile_probe_publishes_stats) {
Vector_Instrumentation::instance.clear();

bool received_vector_max_size_histogram_by_owner_event = false;
bool received_vector_profile_entry = false;
auto on_trace_event =
[&](Debug_Server &server, const Parsed_Trace_Event &event,
[[maybe_unused]] std::uint64_t thread_index) -> bool {
Expand All @@ -245,27 +249,28 @@ TEST_F(Test_Debug_Server, vector_profile_probe_publishes_stats) {

case Parsed_Trace_Event_Type::vector_max_size_histogram_by_owner_event: {
// This should eventually be called.
if (event.vector_max_size_histogram_by_owner_event.entries.empty()) {
// We will receive an initial vector_max_size_histogram_by_owner
// event. Ignore it.
return true;
}

// We will receive an initial vector_max_size_histogram_by_owner
// event, and perhaps spurious extra events. Ignore those events.

Span<const Trace_Vector_Max_Size_Histogram_By_Owner_Entry> entries =
event.vector_max_size_histogram_by_owner_event.entries;
EXPECT_EQ(entries.size(), 1);
if (entries.size() > 0) {
EXPECT_EQ(entries[0].owner, u8"debug server test vector"_sv);
EXPECT_THAT(entries[0].max_size_entries,
::testing::ElementsAreArray({
Trace_Vector_Max_Size_Histogram_Entry{.max_size = 2,
.count = 1},
Trace_Vector_Max_Size_Histogram_Entry{.max_size = 4,
.count = 1},
}));
for (const Trace_Vector_Max_Size_Histogram_By_Owner_Entry &entry :
entries) {
if (entry.owner == u8"debug server test vector"_sv) {
received_vector_profile_entry = true;
EXPECT_EQ(entry.owner, u8"debug server test vector"_sv);
EXPECT_THAT(entry.max_size_entries,
::testing::ElementsAreArray({
Trace_Vector_Max_Size_Histogram_Entry{.max_size = 2,
.count = 1},
Trace_Vector_Max_Size_Histogram_Entry{.max_size = 4,
.count = 1},
}));
}
}

received_vector_max_size_histogram_by_owner_event = true;
received_vector_profile_entry = true;
return false;
}

Expand All @@ -289,7 +294,7 @@ TEST_F(Test_Debug_Server, vector_profile_probe_publishes_stats) {
QLJS_UNREACHABLE();
};
test_web_socket(on_trace_event);
EXPECT_TRUE(received_vector_max_size_histogram_by_owner_event);
EXPECT_TRUE(received_vector_profile_entry);
}
#endif

Expand Down

0 comments on commit 402e689

Please sign in to comment.