From f7e17e55f48b09f8ef12bef07d1dd0e5bcca8112 Mon Sep 17 00:00:00 2001 From: Jason Dellaluce Date: Fri, 28 Jun 2024 17:18:32 +0000 Subject: [PATCH 1/2] refactor(userspace/libsinsp): reduce cost of shared ptrs Signed-off-by: Jason Dellaluce --- userspace/libsinsp/cgroup_limits.cpp | 2 +- userspace/libsinsp/container.cpp | 7 +-- userspace/libsinsp/container.h | 2 +- userspace/libsinsp/container_info.cpp | 5 +-- userspace/libsinsp/container_info.h | 2 +- userspace/libsinsp/event.h | 8 ++-- userspace/libsinsp/eventformatter.h | 2 +- userspace/libsinsp/fdinfo.cpp | 10 ++--- userspace/libsinsp/fdinfo.h | 7 +-- userspace/libsinsp/filter.cpp | 10 ++--- userspace/libsinsp/filter.h | 14 +++--- userspace/libsinsp/metrics_collector.cpp | 2 +- userspace/libsinsp/metrics_collector.h | 2 +- userspace/libsinsp/plugin.cpp | 2 +- userspace/libsinsp/plugin.h | 2 +- userspace/libsinsp/plugin_filtercheck.cpp | 2 +- userspace/libsinsp/plugin_filtercheck.h | 2 +- userspace/libsinsp/plugin_manager.h | 13 +++--- userspace/libsinsp/plugin_table_api.cpp | 12 +++--- userspace/libsinsp/sinsp.cpp | 5 --- userspace/libsinsp/sinsp.h | 11 ++--- .../libsinsp/sinsp_filtercheck_fspath.cpp | 10 ++--- userspace/libsinsp/sinsp_filtercheck_fspath.h | 10 ++--- .../libsinsp/sinsp_filtercheck_gen_event.cpp | 6 +-- userspace/libsinsp/state/table.h | 4 +- userspace/libsinsp/threadinfo.cpp | 43 ++++++++----------- userspace/libsinsp/threadinfo.h | 24 ++++++----- 27 files changed, 110 insertions(+), 109 deletions(-) diff --git a/userspace/libsinsp/cgroup_limits.cpp b/userspace/libsinsp/cgroup_limits.cpp index 187fbbf0f5..4e79ecc633 100644 --- a/userspace/libsinsp/cgroup_limits.cpp +++ b/userspace/libsinsp/cgroup_limits.cpp @@ -68,7 +68,7 @@ bool read_cgroup_vals(const std::string &path, std::istream &stream, int64_t &ou * reasonable being [0; CGROUP_VAL_MAX) */ template -bool read_cgroup_val(std::shared_ptr &subsys, +bool read_cgroup_val(const std::shared_ptr &subsys, const std::string &cgroup, const std::string &filename, int64_t &out, diff --git a/userspace/libsinsp/container.cpp b/userspace/libsinsp/container.cpp index 1ea31a1a78..d1281328ca 100644 --- a/userspace/libsinsp/container.cpp +++ b/userspace/libsinsp/container.cpp @@ -278,7 +278,7 @@ std::string sinsp_container_manager::container_to_json(const sinsp_container_inf return Json::FastWriter().write(obj); } -bool sinsp_container_manager::container_to_sinsp_event(const std::string& json, sinsp_evt* evt, std::shared_ptr tinfo, char *scap_err) +bool sinsp_container_manager::container_to_sinsp_event(const std::string& json, sinsp_evt* evt, std::unique_ptr tinfo, char *scap_err) { uint32_t json_len = json.length() + 1; size_t totlen = sizeof(scap_evt) + sizeof(uint32_t) + json_len; @@ -300,8 +300,9 @@ bool sinsp_container_manager::container_to_sinsp_event(const std::string& json, } evt->init(); - evt->set_tinfo_ref(tinfo); - evt->set_tinfo(tinfo.get()); + std::shared_ptr stinfo = std::move(tinfo); + evt->set_tinfo_ref(stinfo); + evt->set_tinfo(stinfo.get()); return true; } diff --git a/userspace/libsinsp/container.h b/userspace/libsinsp/container.h index 1301acc79b..bbba7de230 100644 --- a/userspace/libsinsp/container.h +++ b/userspace/libsinsp/container.h @@ -235,7 +235,7 @@ class sinsp_container_manager : private: - bool container_to_sinsp_event(const std::string& json, sinsp_evt* evt, std::shared_ptr tinfo, char* scap_err); + bool container_to_sinsp_event(const std::string& json, sinsp_evt* evt, std::unique_ptr tinfo, char* scap_err); std::string get_docker_env(const Json::Value &env_vars, const std::string &mti); std::list> m_container_engines; diff --git a/userspace/libsinsp/container_info.cpp b/userspace/libsinsp/container_info.cpp index 21bd9caef4..4b4c5ed102 100644 --- a/userspace/libsinsp/container_info.cpp +++ b/userspace/libsinsp/container_info.cpp @@ -148,9 +148,9 @@ const sinsp_container_info::container_mount_info *sinsp_container_info::mount_by return NULL; } -std::shared_ptr sinsp_container_info::get_tinfo(sinsp* inspector) const +std::unique_ptr sinsp_container_info::get_tinfo(sinsp* inspector) const { - std::shared_ptr tinfo(inspector->build_threadinfo().release()); + auto tinfo = inspector->build_threadinfo(); tinfo->m_tid = -1; tinfo->m_pid = -1; tinfo->m_vtid = -2; @@ -158,7 +158,6 @@ std::shared_ptr sinsp_container_info::get_tinfo(sinsp* inspect tinfo->m_comm = "container:" + m_id; tinfo->m_exe = "container:" + m_id; tinfo->m_container_id = m_id; - return tinfo; } diff --git a/userspace/libsinsp/container_info.h b/userspace/libsinsp/container_info.h index a292c9f107..6968fa147b 100644 --- a/userspace/libsinsp/container_info.h +++ b/userspace/libsinsp/container_info.h @@ -300,7 +300,7 @@ class sinsp_container_info return m_lookup.get_status(); } - std::shared_ptr get_tinfo(sinsp* inspector) const; + std::unique_ptr get_tinfo(sinsp* inspector) const; // Match a process against the set of health probes container_health_probe::probe_type match_health_probe(sinsp_threadinfo *tinfo) const; diff --git a/userspace/libsinsp/event.h b/userspace/libsinsp/event.h index 0cff2b5470..e65bfdfae7 100644 --- a/userspace/libsinsp/event.h +++ b/userspace/libsinsp/event.h @@ -764,12 +764,12 @@ class SINSP_PUBLIC sinsp_evt return m_tinfo_ref; } - inline std::shared_ptr get_tinfo_ref() + inline const std::shared_ptr& get_tinfo_ref() { return m_tinfo_ref; } - inline void set_tinfo_ref(std::shared_ptr v) + inline void set_tinfo_ref(const std::shared_ptr& v) { m_tinfo_ref = v; } @@ -794,12 +794,12 @@ class SINSP_PUBLIC sinsp_evt return m_fdinfo_ref; } - inline std::shared_ptr get_fdinfo_ref() + inline const std::shared_ptr& get_fdinfo_ref() { return m_fdinfo_ref; } - inline void set_fdinfo_ref(std::shared_ptr v) + inline void set_fdinfo_ref(const std::shared_ptr& v) { m_fdinfo_ref = v; } diff --git a/userspace/libsinsp/eventformatter.h b/userspace/libsinsp/eventformatter.h index 001e4eef99..21ed0c0bf2 100644 --- a/userspace/libsinsp/eventformatter.h +++ b/userspace/libsinsp/eventformatter.h @@ -127,7 +127,7 @@ class SINSP_PUBLIC sinsp_evt_formatter bool has_transformers = false; resolution_token(const std::string& n, token_t t, bool h) - : name(n), token(t), has_transformers(h) { } + : name(n), token(std::move(t)), has_transformers(h) { } }; output_format m_output_format; diff --git a/userspace/libsinsp/fdinfo.cpp b/userspace/libsinsp/fdinfo.cpp index b96a7e6762..a39e1d402d 100644 --- a/userspace/libsinsp/fdinfo.cpp +++ b/userspace/libsinsp/fdinfo.cpp @@ -123,7 +123,7 @@ const char* sinsp_fdinfo::get_typestring() const } } -sinsp_fdinfo::sinsp_fdinfo(std::shared_ptr dyn_fields) +sinsp_fdinfo::sinsp_fdinfo(const std::shared_ptr& dyn_fields) : table_entry(dyn_fields) { } @@ -309,7 +309,7 @@ sinsp_fdtable::sinsp_fdtable(sinsp* inspector) reset_cache(); } -inline std::shared_ptr sinsp_fdtable::find_ref(int64_t fd) +inline const std::shared_ptr& sinsp_fdtable::find_ref(int64_t fd) { // // Try looking up in our simple cache @@ -334,7 +334,7 @@ inline std::shared_ptr sinsp_fdtable::find_ref(int64_t fd) { m_sinsp_stats_v2->m_n_failed_fd_lookups++; } - return nullptr; + return m_nullptr_ret; } else { @@ -350,7 +350,7 @@ inline std::shared_ptr sinsp_fdtable::find_ref(int64_t fd) } } -inline std::shared_ptr sinsp_fdtable::add_ref(int64_t fd, std::unique_ptr fdinfo) +inline const std::shared_ptr& sinsp_fdtable::add_ref(int64_t fd, std::unique_ptr fdinfo) { if (fdinfo->dynamic_fields() != dynamic_fields()) { @@ -386,7 +386,7 @@ inline std::shared_ptr sinsp_fdtable::add_ref(int64_t fd, std::uni } else { - return nullptr; + return m_nullptr_ret; } } else diff --git a/userspace/libsinsp/fdinfo.h b/userspace/libsinsp/fdinfo.h index b3c42e52e5..809942df11 100644 --- a/userspace/libsinsp/fdinfo.h +++ b/userspace/libsinsp/fdinfo.h @@ -111,7 +111,7 @@ class SINSP_PUBLIC sinsp_fdinfo : public libsinsp::state::table_entry FLAGS_CONNECTION_FAILED = (1 << 16), }; - sinsp_fdinfo(std::shared_ptr dyn_fields = nullptr); + sinsp_fdinfo(const std::shared_ptr& dyn_fields = nullptr); sinsp_fdinfo(sinsp_fdinfo&& o) = default; sinsp_fdinfo& operator=(sinsp_fdinfo&& o) = default; sinsp_fdinfo(const sinsp_fdinfo& o) = default; @@ -558,9 +558,10 @@ class sinsp_fdtable : public libsinsp::state::table int64_t m_last_accessed_fd; std::shared_ptr m_last_accessed_fdinfo; uint64_t m_tid; + std::shared_ptr m_nullptr_ret; // needed for returning a reference private: inline void lookup_device(sinsp_fdinfo* fdi, uint64_t fd); - std::shared_ptr find_ref(int64_t fd); - std::shared_ptr add_ref(int64_t fd, std::unique_ptr fdinfo); + const std::shared_ptr& find_ref(int64_t fd); + const std::shared_ptr& add_ref(int64_t fd, std::unique_ptr fdinfo); }; diff --git a/userspace/libsinsp/filter.cpp b/userspace/libsinsp/filter.cpp index 4b14919513..7a86f11e10 100644 --- a/userspace/libsinsp/filter.cpp +++ b/userspace/libsinsp/filter.cpp @@ -188,7 +188,7 @@ void sinsp_filter::add_check(std::unique_ptr chk) sinsp_filter_compiler::sinsp_filter_compiler( sinsp* inspector, const std::string& fltstr, - std::shared_ptr cache_factory) + const std::shared_ptr& cache_factory) : m_flt_str(fltstr), m_factory(std::make_shared(inspector, m_default_filterlist)), m_cache_factory(cache_factory) @@ -196,9 +196,9 @@ sinsp_filter_compiler::sinsp_filter_compiler( } sinsp_filter_compiler::sinsp_filter_compiler( - std::shared_ptr factory, + const std::shared_ptr& factory, const std::string& fltstr, - std::shared_ptr cache_factory) + const std::shared_ptr& cache_factory) : m_flt_str(fltstr), m_factory(factory), m_cache_factory(cache_factory) @@ -206,9 +206,9 @@ sinsp_filter_compiler::sinsp_filter_compiler( } sinsp_filter_compiler::sinsp_filter_compiler( - std::shared_ptr factory, + const std::shared_ptr& factory, const libsinsp::filter::ast::expr* fltast, - std::shared_ptr cache_factory) + const std::shared_ptr& cache_factory) : m_flt_ast(fltast), m_factory(factory), m_cache_factory(cache_factory) diff --git a/userspace/libsinsp/filter.h b/userspace/libsinsp/filter.h index 58d82e255e..f8e18d95d2 100644 --- a/userspace/libsinsp/filter.h +++ b/userspace/libsinsp/filter.h @@ -202,7 +202,7 @@ class SINSP_PUBLIC sinsp_filter_compiler: sinsp_filter_compiler( sinsp* inspector, const std::string& fltstr, - std::shared_ptr cache_factory = nullptr); + const std::shared_ptr& cache_factory = nullptr); /*! \brief Constructs the compiler @@ -212,9 +212,9 @@ class SINSP_PUBLIC sinsp_filter_compiler: \param fltstr The filter string to compile */ sinsp_filter_compiler( - std::shared_ptr factory, + const std::shared_ptr& factory, const std::string& fltstr, - std::shared_ptr cache_factory = nullptr); + const std::shared_ptr& cache_factory = nullptr); /*! \brief Constructs the compiler @@ -225,9 +225,9 @@ class SINSP_PUBLIC sinsp_filter_compiler: tree */ sinsp_filter_compiler( - std::shared_ptr factory, + const std::shared_ptr& factory, const libsinsp::filter::ast::expr* fltast, - std::shared_ptr cache_factory = nullptr); + const std::shared_ptr& cache_factory = nullptr); /*! \brief Builds a filtercheck tree and bundles it in sinsp_filter @@ -237,9 +237,9 @@ class SINSP_PUBLIC sinsp_filter_compiler: */ std::unique_ptr compile(); - std::shared_ptr get_filter_ast() const { return m_internal_flt_ast; } + const std::shared_ptr get_filter_ast() const { return m_internal_flt_ast; } - std::shared_ptr get_filter_ast() { return m_internal_flt_ast; } + const std::shared_ptr& get_filter_ast() { return m_internal_flt_ast; } const libsinsp::filter::ast::pos_info& get_pos() const { return m_pos; } diff --git a/userspace/libsinsp/metrics_collector.cpp b/userspace/libsinsp/metrics_collector.cpp index 8ed3e0f06b..f8872f61d1 100644 --- a/userspace/libsinsp/metrics_collector.cpp +++ b/userspace/libsinsp/metrics_collector.cpp @@ -508,7 +508,7 @@ void libs_resource_utilization::get_container_memory_used() fclose(f); } -libs_state_counters::libs_state_counters(std::shared_ptr sinsp_stats_v2, sinsp_thread_manager* thread_manager) : m_sinsp_stats_v2(sinsp_stats_v2), m_n_fds(0), m_n_threads(0) { +libs_state_counters::libs_state_counters(const std::shared_ptr& sinsp_stats_v2, sinsp_thread_manager* thread_manager) : m_sinsp_stats_v2(sinsp_stats_v2), m_n_fds(0), m_n_threads(0) { if (thread_manager != nullptr) { m_n_threads = thread_manager->get_thread_count(); diff --git a/userspace/libsinsp/metrics_collector.h b/userspace/libsinsp/metrics_collector.h index b60643718c..a5077c4c43 100644 --- a/userspace/libsinsp/metrics_collector.h +++ b/userspace/libsinsp/metrics_collector.h @@ -303,7 +303,7 @@ class libs_resource_utilization : libsinsp_metrics class libs_state_counters : libsinsp_metrics { public: - libs_state_counters(std::shared_ptr sinsp_stats_v2, sinsp_thread_manager* thread_manager); + libs_state_counters(const std::shared_ptr& sinsp_stats_v2, sinsp_thread_manager* thread_manager); std::vector to_metrics() override; diff --git a/userspace/libsinsp/plugin.cpp b/userspace/libsinsp/plugin.cpp index 76aa9877f4..44a932781b 100755 --- a/userspace/libsinsp/plugin.cpp +++ b/userspace/libsinsp/plugin.cpp @@ -958,7 +958,7 @@ std::vector sinsp_plugin::get_metrics() const /** Field Extraction CAP **/ -std::unique_ptr sinsp_plugin::new_filtercheck(std::shared_ptr plugin) +std::unique_ptr sinsp_plugin::new_filtercheck(const std::shared_ptr& plugin) { return std::make_unique(plugin); } diff --git a/userspace/libsinsp/plugin.h b/userspace/libsinsp/plugin.h index 49f2e3784f..e9972be73b 100755 --- a/userspace/libsinsp/plugin.h +++ b/userspace/libsinsp/plugin.h @@ -80,7 +80,7 @@ class sinsp_plugin * * todo(jasondellaluce): make this return a unique_ptr */ - static std::unique_ptr new_filtercheck(std::shared_ptr plugin); + static std::unique_ptr new_filtercheck(const std::shared_ptr& plugin); /** * @brief Returns true if the source is compatible with the given set diff --git a/userspace/libsinsp/plugin_filtercheck.cpp b/userspace/libsinsp/plugin_filtercheck.cpp index 37bf44f877..8f10751345 100755 --- a/userspace/libsinsp/plugin_filtercheck.cpp +++ b/userspace/libsinsp/plugin_filtercheck.cpp @@ -27,7 +27,7 @@ sinsp_filter_check_plugin::sinsp_filter_check_plugin() m_eplugin = nullptr; } -sinsp_filter_check_plugin::sinsp_filter_check_plugin(std::shared_ptr plugin) +sinsp_filter_check_plugin::sinsp_filter_check_plugin(const std::shared_ptr& plugin) { if (!(plugin->caps() & CAP_EXTRACTION)) { diff --git a/userspace/libsinsp/plugin_filtercheck.h b/userspace/libsinsp/plugin_filtercheck.h index 7f850a2220..55adc4c478 100755 --- a/userspace/libsinsp/plugin_filtercheck.h +++ b/userspace/libsinsp/plugin_filtercheck.h @@ -35,7 +35,7 @@ class sinsp_filter_check_plugin : public sinsp_filter_check public: sinsp_filter_check_plugin(); - explicit sinsp_filter_check_plugin(std::shared_ptr plugin); + explicit sinsp_filter_check_plugin(const std::shared_ptr& plugin); explicit sinsp_filter_check_plugin(const sinsp_filter_check_plugin &p); diff --git a/userspace/libsinsp/plugin_manager.h b/userspace/libsinsp/plugin_manager.h index 667f814933..a635ab86a8 100755 --- a/userspace/libsinsp/plugin_manager.h +++ b/userspace/libsinsp/plugin_manager.h @@ -49,7 +49,7 @@ class sinsp_plugin_manager /** * @brief Adds a plugin in the manager. */ - void add(std::shared_ptr plugin) + void add(const std::shared_ptr& plugin) { for(auto& it : m_plugins) { @@ -117,14 +117,14 @@ class sinsp_plugin_manager * the CAP_EVENT_SOURCE capability. Returns nullptr if no plugin exists * with the given ID. */ - inline std::shared_ptr plugin_by_id(uint32_t plugin_id) const + inline const std::shared_ptr& plugin_by_id(uint32_t plugin_id) const { if (plugin_id != m_last_id_in) { auto it = m_plugins_id_index.find(plugin_id); if(it == m_plugins_id_index.end()) { - return nullptr; + return m_nullptr_ret; } m_last_id_in = plugin_id; m_last_id_out = it->second; @@ -136,13 +136,13 @@ class sinsp_plugin_manager * @brief Returns a plugin given an event. The plugin is guaranteed to have * the CAP_EVENT_SOURCE capability. */ - inline std::shared_ptr plugin_by_evt(sinsp_evt* evt) const + inline const std::shared_ptr& plugin_by_evt(sinsp_evt* evt) const { if(evt && evt->get_type() == PPME_PLUGINEVENT_E) { return plugin_by_id(evt->get_param(0)->as()); } - return nullptr; + return m_nullptr_ret; } /** @@ -186,4 +186,7 @@ class sinsp_plugin_manager mutable size_t m_last_id_out; mutable size_t m_last_source_in; mutable size_t m_last_source_out; + + /* Used internally just for the sake of returning a reference */ + const std::shared_ptr m_nullptr_ret; }; diff --git a/userspace/libsinsp/plugin_table_api.cpp b/userspace/libsinsp/plugin_table_api.cpp index ce0ea573c5..ec62b4cdc5 100755 --- a/userspace/libsinsp/plugin_table_api.cpp +++ b/userspace/libsinsp/plugin_table_api.cpp @@ -479,7 +479,8 @@ struct plugin_table_wrapper: public libsinsp::state::table : libsinsp::state::table(i->name, &s_empty_static_infos), m_owner(o), m_input(copy_and_check_table_input(o, i)), - m_dyn_fields(std::make_shared(o, m_input)) + m_dyn_fields(std::make_shared(o, m_input)), + m_dyn_fields_as_base_class(m_dyn_fields) { auto t = libsinsp::state::typeinfo::of(); if (m_input->key_type != typeinfo_to_state_type(t)) @@ -497,6 +498,7 @@ struct plugin_table_wrapper: public libsinsp::state::table sinsp_plugin* m_owner; owned_table_input_t m_input; std::shared_ptr m_dyn_fields; + std::shared_ptr m_dyn_fields_as_base_class; const libsinsp::state::static_struct::field_infos* static_fields() const override { @@ -505,9 +507,9 @@ struct plugin_table_wrapper: public libsinsp::state::table return &s_empty_static_infos; } - std::shared_ptr dynamic_fields() const override + const std::shared_ptr& dynamic_fields() const override { - return m_dyn_fields; + return m_dyn_fields_as_base_class; } size_t entries_count() const override @@ -577,7 +579,7 @@ struct plugin_table_wrapper: public libsinsp::state::table { throw sinsp_exception(table_input_error_prefix(m_owner, m_input.get()) + "create entry failure: " + m_owner->get_last_error()); } - return std::unique_ptr(new plugin_table_entry(m_owner, m_input, m_dyn_fields, res, true)); + return std::make_unique(m_owner, m_input, m_dyn_fields, res, true); } std::shared_ptr get_entry(const KeyType& key) override @@ -596,7 +598,7 @@ struct plugin_table_wrapper: public libsinsp::state::table // critical path, however it should be used only when doing a sinsp->plugin // access, which is expected to not be common. For plugin->plugin table // access, we optimize for invoking the plugin's table symbol right away - return std::shared_ptr(new plugin_table_entry(m_owner, m_input, m_dyn_fields, res, false)); + return std::make_shared(m_owner, m_input, m_dyn_fields, res, false); } std::shared_ptr add_entry(const KeyType& key, std::unique_ptr e) override diff --git a/userspace/libsinsp/sinsp.cpp b/userspace/libsinsp/sinsp.cpp index 51c6a211cc..025a6b254a 100644 --- a/userspace/libsinsp/sinsp.cpp +++ b/userspace/libsinsp/sinsp.cpp @@ -1691,11 +1691,6 @@ std::string sinsp::get_filter() const return m_filterstring; } -std::shared_ptr sinsp::get_filter_ast() -{ - return m_internal_flt_ast; -} - bool sinsp::run_filters_on_evt(sinsp_evt *evt) { // diff --git a/userspace/libsinsp/sinsp.h b/userspace/libsinsp/sinsp.h index de9c394f31..8fed6f9cda 100644 --- a/userspace/libsinsp/sinsp.h +++ b/userspace/libsinsp/sinsp.h @@ -156,8 +156,6 @@ enum sinsp_mode_t class SINSP_PUBLIC sinsp : public capture_stats_source { public: - typedef std::shared_ptr ptr; - sinsp(bool static_container = false, const std::string &static_id = "", const std::string &static_name = "", @@ -322,7 +320,10 @@ class SINSP_PUBLIC sinsp : public capture_stats_source \return the AST (wrapped in a shared pointer) corresponding to the filter previously set with \ref set_filter().. */ - std::shared_ptr get_filter_ast(); + inline const std::shared_ptr& get_filter_ast() + { + return m_internal_flt_ast; + } bool run_filters_on_evt(sinsp_evt *evt); @@ -470,7 +471,7 @@ class SINSP_PUBLIC sinsp : public capture_stats_source \brief Return sinsp stats v2 containing continually updated counters around thread and fd state tables. */ - inline std::shared_ptr get_sinsp_stats_v2() + inline const std::shared_ptr& get_sinsp_stats_v2() { return m_sinsp_stats_v2; } @@ -1027,7 +1028,7 @@ class SINSP_PUBLIC sinsp : public capture_stats_source bool remove_inactive_threads(); - inline std::shared_ptr add_thread(std::unique_ptr ptinfo) + inline const std::shared_ptr& add_thread(std::unique_ptr ptinfo) { return m_thread_manager->add_thread(std::move(ptinfo), false); } diff --git a/userspace/libsinsp/sinsp_filtercheck_fspath.cpp b/userspace/libsinsp/sinsp_filtercheck_fspath.cpp index af285fe6d9..dbe5a1ec2e 100644 --- a/userspace/libsinsp/sinsp_filtercheck_fspath.cpp +++ b/userspace/libsinsp/sinsp_filtercheck_fspath.cpp @@ -212,10 +212,10 @@ void sinsp_filter_check_fspath::create_fspath_checks() m_success_checks->emplace(PPME_SYSCALL_UMOUNT2_X, evt_arg_res_eq_0); } -void sinsp_filter_check_fspath::set_fspath_checks(std::shared_ptr success_checks, - std::shared_ptr path_checks, - std::shared_ptr source_checks, - std::shared_ptr target_checks) +void sinsp_filter_check_fspath::set_fspath_checks(const std::shared_ptr& success_checks, + const std::shared_ptr& path_checks, + const std::shared_ptr& source_checks, + const std::shared_ptr& target_checks) { m_success_checks = success_checks; m_path_checks = path_checks; @@ -368,7 +368,7 @@ uint8_t* sinsp_filter_check_fspath::extract_single(sinsp_evt* evt, uint32_t* len bool sinsp_filter_check_fspath::extract_fspath(sinsp_evt* evt, std::vector& values, - std::shared_ptr checks) + const std::shared_ptr& checks) { sinsp_evt* extract_evt = evt; diff --git a/userspace/libsinsp/sinsp_filtercheck_fspath.h b/userspace/libsinsp/sinsp_filtercheck_fspath.h index d2112074aa..91edf76b04 100644 --- a/userspace/libsinsp/sinsp_filtercheck_fspath.h +++ b/userspace/libsinsp/sinsp_filtercheck_fspath.h @@ -51,13 +51,13 @@ class sinsp_filter_check_fspath : public sinsp_filter_check std::shared_ptr create_fd_check(const char *name); void create_fspath_checks(); - void set_fspath_checks(std::shared_ptr success_checks, - std::shared_ptr path_checks, - std::shared_ptr source_checks, - std::shared_ptr target_checks); + void set_fspath_checks(const std::shared_ptr& success_checks, + const std::shared_ptr& path_checks, + const std::shared_ptr& source_checks, + const std::shared_ptr& target_checks); bool extract_fspath(sinsp_evt* evt, std::vector& values, - std::shared_ptr map); + const std::shared_ptr& map); std::string m_tstr; std::shared_ptr m_success_checks; diff --git a/userspace/libsinsp/sinsp_filtercheck_gen_event.cpp b/userspace/libsinsp/sinsp_filtercheck_gen_event.cpp index 4dbb60dab8..ed2fa4b25a 100644 --- a/userspace/libsinsp/sinsp_filtercheck_gen_event.cpp +++ b/userspace/libsinsp/sinsp_filtercheck_gen_event.cpp @@ -111,8 +111,6 @@ Json::Value sinsp_filter_check_gen_event::extract_as_js(sinsp_evt *evt, uint32_t uint8_t* sinsp_filter_check_gen_event::extract_single(sinsp_evt *evt, uint32_t* len, bool sanitize_strings) { - - std::shared_ptr plugin; const scap_machine_info* minfo; *len = 0; @@ -163,7 +161,8 @@ uint8_t* sinsp_filter_check_gen_event::extract_single(sinsp_evt *evt, uint32_t* RETURN_EXTRACT_VAR(m_val.u64); case TYPE_PLUGINNAME: case TYPE_PLUGININFO: - plugin = m_inspector->get_plugin_manager()->plugin_by_evt(evt); + { + const auto& plugin = m_inspector->get_plugin_manager()->plugin_by_evt(evt); if (plugin == nullptr) { return NULL; @@ -179,6 +178,7 @@ uint8_t* sinsp_filter_check_gen_event::extract_single(sinsp_evt *evt, uint32_t* } RETURN_EXTRACT_STRING(m_strstorage); + } case TYPE_SOURCE: if (evt->get_source_idx() == sinsp_no_event_source_idx || evt->get_source_name() == sinsp_no_event_source_name) diff --git a/userspace/libsinsp/state/table.h b/userspace/libsinsp/state/table.h index d548e69e7c..87c00d15e5 100644 --- a/userspace/libsinsp/state/table.h +++ b/userspace/libsinsp/state/table.h @@ -105,12 +105,12 @@ class base_table * be allocated and accessible for all the present and future entries * present in the table. */ - virtual std::shared_ptr dynamic_fields() const + virtual const std::shared_ptr& dynamic_fields() const { return m_dynamic_fields; } - virtual void set_dynamic_fields(std::shared_ptr dynf) + virtual void set_dynamic_fields(const std::shared_ptr& dynf) { if (!dynf) { diff --git a/userspace/libsinsp/threadinfo.cpp b/userspace/libsinsp/threadinfo.cpp index a856098d77..6cf2577b18 100644 --- a/userspace/libsinsp/threadinfo.cpp +++ b/userspace/libsinsp/threadinfo.cpp @@ -44,7 +44,7 @@ static void copy_ipv6_address(uint32_t* dest, uint32_t* src) // sinsp_threadinfo implementation /////////////////////////////////////////////////////////////////////////////// -sinsp_threadinfo::sinsp_threadinfo(sinsp* inspector, std::shared_ptr dyn_fields): +sinsp_threadinfo::sinsp_threadinfo(sinsp* inspector, const std::shared_ptr& dyn_fields): table_entry(dyn_fields), m_cgroups(new cgroups_t), m_inspector(inspector), @@ -1493,7 +1493,7 @@ std::unique_ptr sinsp_thread_manager::new_fdinfo() const * 2. We are doing a proc scan with a callback or without. (`from_scap_proctable==true`) * 3. We are trying to obtain thread info from /proc through `get_thread_ref` */ -std::shared_ptr sinsp_thread_manager::add_thread(std::unique_ptr threadinfo, bool from_scap_proctable) +const std::shared_ptr& sinsp_thread_manager::add_thread(std::unique_ptr threadinfo, bool from_scap_proctable) { /* We have no more space */ @@ -1511,7 +1511,8 @@ std::shared_ptr sinsp_thread_manager::add_thread(std::unique_p } m_sinsp_stats_v2->m_n_drops_full_threadtable++; } - return nullptr; + + return m_nullptr_tinfo_ret; } auto tinfo_shared_ptr = std::shared_ptr(std::move(threadinfo)); @@ -1532,14 +1533,12 @@ std::shared_ptr sinsp_thread_manager::add_thread(std::unique_p } tinfo_shared_ptr->compute_program_hash(); - m_threadtable.put(tinfo_shared_ptr); - if (m_sinsp_stats_v2 != nullptr) { m_sinsp_stats_v2->m_n_added_threads++; } - return tinfo_shared_ptr; + return m_threadtable.put(tinfo_shared_ptr); } /* Taken from `find_new_reaper` kernel function: @@ -2037,9 +2036,9 @@ void sinsp_thread_manager::dump_threads_to_file(scap_dumper_t* dumper) }); } -threadinfo_map_t::ptr_t sinsp_thread_manager::get_thread_ref(int64_t tid, bool query_os_if_not_found, bool lookup_only, bool main_thread) +const threadinfo_map_t::ptr_t& sinsp_thread_manager::get_thread_ref(int64_t tid, bool query_os_if_not_found, bool lookup_only, bool main_thread) { - auto sinsp_proc = find_thread(tid, lookup_only); + const auto& sinsp_proc = find_thread(tid, lookup_only); if(!sinsp_proc && query_os_if_not_found && (m_threadtable.size() < m_max_thread_table_size || tid == m_inspector->m_self_pid)) @@ -2130,39 +2129,34 @@ threadinfo_map_t::ptr_t sinsp_thread_manager::get_thread_ref(int64_t tid, bool q // Done. Add the new thread to the list. // add_thread(std::move(newti), false); - sinsp_proc = find_thread(tid, lookup_only); + return find_thread(tid, lookup_only); } return sinsp_proc; } /* `lookup_only==true` means that we don't fill the `m_last_tinfo` field */ -threadinfo_map_t::ptr_t sinsp_thread_manager::find_thread(int64_t tid, bool lookup_only) +const threadinfo_map_t::ptr_t& sinsp_thread_manager::find_thread(int64_t tid, bool lookup_only) { - threadinfo_map_t::ptr_t thr; // // Try looking up in our simple cache // - if(tid == m_last_tid) + if(tid == m_last_tid && m_last_tinfo) { - thr = m_last_tinfo.lock(); - if (thr) + if (m_sinsp_stats_v2 != nullptr) { - if (m_sinsp_stats_v2 != nullptr) - { - m_sinsp_stats_v2->m_n_cached_thread_lookups++; - } - // This allows us to avoid performing an actual timestamp lookup - // for something that may not need to be precise - thr->m_lastaccess_ts = m_inspector->get_lastevent_ts(); - return thr; + m_sinsp_stats_v2->m_n_cached_thread_lookups++; } + // This allows us to avoid performing an actual timestamp lookup + // for something that may not need to be precise + m_last_tinfo->m_lastaccess_ts = m_inspector->get_lastevent_ts(); + return m_last_tinfo; } // // Caching failed, do a real lookup // - thr = m_threadtable.get_ref(tid); + const auto& thr = m_threadtable.get_ref(tid); if(thr) { @@ -2185,7 +2179,8 @@ threadinfo_map_t::ptr_t sinsp_thread_manager::find_thread(int64_t tid, bool look { m_sinsp_stats_v2->m_n_failed_thread_lookups++; } - return NULL; + + return m_nullptr_tinfo_ret; } } diff --git a/userspace/libsinsp/threadinfo.h b/userspace/libsinsp/threadinfo.h index ec428de4da..08dd68381d 100644 --- a/userspace/libsinsp/threadinfo.h +++ b/userspace/libsinsp/threadinfo.h @@ -138,7 +138,7 @@ class SINSP_PUBLIC sinsp_threadinfo : public libsinsp::state::table_entry sinsp_threadinfo( sinsp *inspector = nullptr, - std::shared_ptr dyn_fields = nullptr); + const std::shared_ptr& dyn_fields = nullptr); virtual ~sinsp_threadinfo(); libsinsp::state::static_struct::field_infos static_fields() const override; @@ -754,9 +754,10 @@ class threadinfo_map_t typedef std::function visitor_t; typedef std::shared_ptr ptr_t; - inline void put(ptr_t tinfo) + inline const ptr_t& put(const ptr_t& tinfo) { m_threads[tinfo->m_tid] = tinfo; + return m_threads[tinfo->m_tid]; } inline sinsp_threadinfo* get(uint64_t tid) @@ -769,12 +770,12 @@ class threadinfo_map_t return it->second.get(); } - inline ptr_t get_ref(uint64_t tid) + inline const ptr_t& get_ref(uint64_t tid) { auto it = m_threads.find(tid); if (it == m_threads.end()) { - return nullptr; + return m_nullptr_ret; } return it->second; } @@ -832,6 +833,7 @@ class threadinfo_map_t protected: std::unordered_map m_threads; + const ptr_t m_nullptr_ret; // needed for returning a reference }; /////////////////////////////////////////////////////////////////////////////// @@ -850,7 +852,7 @@ class SINSP_PUBLIC sinsp_thread_manager: public libsinsp::state::table void set_tinfo_shared_dynamic_fields(sinsp_threadinfo& tinfo) const; void set_fdinfo_shared_dynamic_fields(sinsp_fdinfo& fdinfo) const; - threadinfo_map_t::ptr_t add_thread(std::unique_ptr threadinfo, bool from_scap_proctable); + const threadinfo_map_t::ptr_t& add_thread(std::unique_ptr threadinfo, bool from_scap_proctable); sinsp_threadinfo* find_new_reaper(sinsp_threadinfo*); void remove_thread(int64_t tid); // Returns true if the table is actually scanned @@ -880,7 +882,7 @@ class SINSP_PUBLIC sinsp_thread_manager: public libsinsp::state::table of failure. */ - threadinfo_map_t::ptr_t get_thread_ref(int64_t tid, bool query_os_if_not_found = false, bool lookup_only = true, bool main_thread=false); + const threadinfo_map_t::ptr_t& get_thread_ref(int64_t tid, bool query_os_if_not_found = false, bool lookup_only = true, bool main_thread=false); // // Note: lookup_only should be used when the query for the thread is made @@ -888,7 +890,7 @@ class SINSP_PUBLIC sinsp_thread_manager: public libsinsp::state::table // just for lookup reason. In that case, m_lastaccess_ts is not updated // and m_last_tinfo is not set. // - threadinfo_map_t::ptr_t find_thread(int64_t tid, bool lookup_only); + const threadinfo_map_t::ptr_t& find_thread(int64_t tid, bool lookup_only); void dump_threads_to_file(scap_dumper_t* dumper); @@ -969,14 +971,14 @@ class SINSP_PUBLIC sinsp_thread_manager: public libsinsp::state::table return false; } - inline std::shared_ptr get_thread_group_info(int64_t pid) const + inline const std::shared_ptr& get_thread_group_info(int64_t pid) const { auto tgroup = m_thread_groups.find(pid); if(tgroup != m_thread_groups.end()) { return tgroup->second; } - return nullptr; + return m_nullptr_tginfo_ret; } inline void set_thread_group_info(int64_t pid, const std::shared_ptr& tginfo) @@ -1022,7 +1024,7 @@ class SINSP_PUBLIC sinsp_thread_manager: public libsinsp::state::table std::unordered_map> m_thread_groups; threadinfo_map_t m_threadtable; int64_t m_last_tid; - std::weak_ptr m_last_tinfo; + std::shared_ptr m_last_tinfo; uint64_t m_last_flush_time_ns; // Increased legacy default of 131072 in January 2024 to prevent // possible drops due to full threadtable on more modern servers @@ -1035,4 +1037,6 @@ class SINSP_PUBLIC sinsp_thread_manager: public libsinsp::state::table int32_t m_max_n_proc_socket_lookups = -1; std::shared_ptr m_fdtable_dyn_fields; + const std::shared_ptr m_nullptr_tinfo_ret; // needed for returning a reference + const std::shared_ptr m_nullptr_tginfo_ret; // needed for returning a reference }; From a147d68d541faf73cb121b79be1c2b1a027bb6a3 Mon Sep 17 00:00:00 2001 From: Jason Dellaluce Date: Wed, 10 Jul 2024 07:47:57 +0000 Subject: [PATCH 2/2] refactor(userspace/libsinsp): pass by reference when possible Signed-off-by: Jason Dellaluce --- userspace/libsinsp/gvisor_config.cpp | 2 +- userspace/libsinsp/gvisor_config.h | 2 +- userspace/libsinsp/logger.cpp | 3 ++- userspace/libsinsp/logger.h | 2 +- userspace/libsinsp/plugin.cpp | 2 +- userspace/libsinsp/plugin.h | 2 +- userspace/libsinsp/sinsp.cpp | 4 ++-- userspace/libsinsp/sinsp.h | 4 ++-- userspace/libsinsp/threadinfo.cpp | 10 +++++----- userspace/libsinsp/threadinfo.h | 4 ++-- userspace/libsinsp/utils.cpp | 2 +- 11 files changed, 19 insertions(+), 18 deletions(-) diff --git a/userspace/libsinsp/gvisor_config.cpp b/userspace/libsinsp/gvisor_config.cpp index 666096920f..3569826610 100644 --- a/userspace/libsinsp/gvisor_config.cpp +++ b/userspace/libsinsp/gvisor_config.cpp @@ -333,7 +333,7 @@ static const std::vector s_gvisor_points = { constexpr unsigned int max_retries = 3; -std::string generate(std::string socket_path) +std::string generate(const std::string& socket_path) { Json::Value jpoints; for(const auto &point_info : s_gvisor_points) diff --git a/userspace/libsinsp/gvisor_config.h b/userspace/libsinsp/gvisor_config.h index 93a90f6bf8..b0d874b9ce 100644 --- a/userspace/libsinsp/gvisor_config.h +++ b/userspace/libsinsp/gvisor_config.h @@ -22,5 +22,5 @@ limitations under the License. namespace gvisor_config { - std::string generate(std::string socket_path); + std::string generate(const std::string& socket_path); } diff --git a/userspace/libsinsp/logger.cpp b/userspace/libsinsp/logger.cpp index 4d4066d555..f6b4b4388f 100644 --- a/userspace/libsinsp/logger.cpp +++ b/userspace/libsinsp/logger.cpp @@ -144,7 +144,7 @@ sinsp_logger::severity sinsp_logger::get_severity() const return m_sev; } -void sinsp_logger::log(std::string msg, const severity sev) +void sinsp_logger::log(const std::string& m, const severity sev) { sinsp_logger_callback cb = nullptr; @@ -153,6 +153,7 @@ void sinsp_logger::log(std::string msg, const severity sev) return; } + std::string msg = m; if((m_flags & sinsp_logger::OT_NOTS) == 0) { struct timeval ts = {}; diff --git a/userspace/libsinsp/logger.h b/userspace/libsinsp/logger.h index 250c7f0303..80425486b1 100644 --- a/userspace/libsinsp/logger.h +++ b/userspace/libsinsp/logger.h @@ -127,7 +127,7 @@ class SINSP_PUBLIC sinsp_logger * Emit the given msg to the configured log sink if the given sev * is greater than or equal to the minimum configured logging severity. */ - void log(std::string msg, severity sev = SEV_INFO); + void log(const std::string& m, severity sev = SEV_INFO); /** * Write the given printf-style log message of the given severity diff --git a/userspace/libsinsp/plugin.cpp b/userspace/libsinsp/plugin.cpp index 44a932781b..3de7dcc657 100755 --- a/userspace/libsinsp/plugin.cpp +++ b/userspace/libsinsp/plugin.cpp @@ -134,7 +134,7 @@ std::shared_ptr sinsp_plugin::create( return plugin; } -bool sinsp_plugin::is_plugin_loaded(std::string &filepath) +bool sinsp_plugin::is_plugin_loaded(const std::string &filepath) { return plugin_is_loaded(filepath.c_str()); } diff --git a/userspace/libsinsp/plugin.h b/userspace/libsinsp/plugin.h index e9972be73b..4f8b546023 100755 --- a/userspace/libsinsp/plugin.h +++ b/userspace/libsinsp/plugin.h @@ -72,7 +72,7 @@ class sinsp_plugin /** * @brief Return whether a filesystem dynamic library object is loaded. */ - static bool is_plugin_loaded(std::string& filepath); + static bool is_plugin_loaded(const std::string& filepath); /** * @brief If the plugin has CAP_EXTRACTION capability, returns a diff --git a/userspace/libsinsp/sinsp.cpp b/userspace/libsinsp/sinsp.cpp index 025a6b254a..86b722df41 100644 --- a/userspace/libsinsp/sinsp.cpp +++ b/userspace/libsinsp/sinsp.cpp @@ -670,7 +670,7 @@ bool sinsp::check_current_engine(const std::string& engine_name) const /*=============================== Engine related ===============================*/ -std::string sinsp::generate_gvisor_config(std::string socket_path) +std::string sinsp::generate_gvisor_config(const std::string& socket_path) { return gvisor_config::generate(socket_path); } @@ -1800,7 +1800,7 @@ void sinsp::set_log_callback(sinsp_logger_callback cb) } } -void sinsp::set_log_file(std::string filename) +void sinsp::set_log_file(const std::string& filename) { libsinsp_logger()->add_file_log(filename); } diff --git a/userspace/libsinsp/sinsp.h b/userspace/libsinsp/sinsp.h index 8fed6f9cda..66632da5ae 100644 --- a/userspace/libsinsp/sinsp.h +++ b/userspace/libsinsp/sinsp.h @@ -184,7 +184,7 @@ class SINSP_PUBLIC sinsp : public capture_stats_source scap_fseek(m_h, filepos); } - std::string generate_gvisor_config(std::string socket_path); + std::string generate_gvisor_config(const std::string& socket_path); /*! @@ -338,7 +338,7 @@ class SINSP_PUBLIC sinsp : public capture_stats_source /*! \brief Instruct sinsp to write its log messages to the given file. */ - void set_log_file(std::string filename); + void set_log_file(const std::string& filename); /*! \brief Instruct sinsp to write its log messages to stderr. diff --git a/userspace/libsinsp/threadinfo.cpp b/userspace/libsinsp/threadinfo.cpp index 6cf2577b18..c1ea6e0dbd 100644 --- a/userspace/libsinsp/threadinfo.cpp +++ b/userspace/libsinsp/threadinfo.cpp @@ -628,7 +628,7 @@ void sinsp_threadinfo::set_args(const char* args, size_t len) set_args(sinsp_split(args, len, '\0')); } -void sinsp_threadinfo::set_args(std::vector args) +void sinsp_threadinfo::set_args(const std::vector& args) { m_args = args; } @@ -742,11 +742,11 @@ void sinsp_threadinfo::set_cgroups(const char* cgroups, size_t len) set_cgroups(sinsp_split(cgroups, len, '\0')); } -void sinsp_threadinfo::set_cgroups(std::vector cgroups) +void sinsp_threadinfo::set_cgroups(const std::vector& cgroups) { - decltype(m_cgroups) tmp_cgroups(new cgroups_t); + auto tmp_cgroups = std::make_unique(); - for( auto &def : cgroups) + for(const auto &def : cgroups) { std::string::size_type eq_pos = def.find("="); if (eq_pos == std::string::npos) @@ -2050,7 +2050,7 @@ const threadinfo_map_t::ptr_t& sinsp_thread_manager::get_thread_ref(int64_t tid, { libsinsp_logger()->format(sinsp_logger::SEV_INFO, "%s: Unable to complete for tid=%" PRIu64 ": sinsp::scap_t* is uninitialized", __func__, tid); - return NULL; + return m_nullptr_tinfo_ret; } scap_threadinfo scap_proc {}; diff --git a/userspace/libsinsp/threadinfo.h b/userspace/libsinsp/threadinfo.h index 08dd68381d..82fa9c3728 100644 --- a/userspace/libsinsp/threadinfo.h +++ b/userspace/libsinsp/threadinfo.h @@ -640,10 +640,10 @@ class SINSP_PUBLIC sinsp_threadinfo : public libsinsp::state::table_entry void remove_fd(int64_t fd); void update_cwd(std::string_view cwd); void set_args(const char* args, size_t len); - void set_args(std::vector args); + void set_args(const std::vector& args); void set_env(const char* env, size_t len); void set_cgroups(const char* cgroups, size_t len); - void set_cgroups(std::vector cgroups); + void set_cgroups(const std::vector& cgroups); bool is_lastevent_data_valid() const; inline void set_lastevent_data_validity(bool isvalid) { diff --git a/userspace/libsinsp/utils.cpp b/userspace/libsinsp/utils.cpp index b14c8209b8..9f0c608d34 100644 --- a/userspace/libsinsp/utils.cpp +++ b/userspace/libsinsp/utils.cpp @@ -869,7 +869,7 @@ void sinsp_utils::split_container_image(const std::string &image, std::string &digest, bool split_repo) { - auto split = [](const std::string &src, std::string &part1, std::string &part2, const std::string sep) + auto split = [](const std::string &src, std::string &part1, std::string &part2, const std::string& sep) { size_t pos = src.find(sep); if(pos != std::string::npos)