Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(userspace/libsinsp): keep a reference on m_sinsp_stats_v2 where needed #1910

Merged
merged 5 commits into from
Jun 25, 2024

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Jun 14, 2024

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area libsinsp

Does this PR require a change in the driver versions?

What this PR does / why we need it:

This PR keeps a reference on m_sinsp_stats_v2 where needed, instead of fetching it every time.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

chore!(userspace/libsinsp): keep a reference on m_sinsp_stats_v2 where needed 

@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 14, 2024

Mmmh seems like the job is hit by: actions/runner#2402

@FedeDP FedeDP force-pushed the perf_improvements branch 3 times, most recently from 67108fd to 574fbaf Compare June 14, 2024 11:00
if (!m_sinsp_stats_v2)
{
// Sinsp stats disabled.
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below we also have other metrics categories that then could not be used without the sinsip stats?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this early return is ok, it has the exact same behavior as before as far as i can tell; otherwise, can you point me to the lines (perhaps i am just missing them!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if((m_metrics_flags & METRICS_V2_RESOURCE_UTILIZATION))
{
const scap_agent_info* agent_info = m_inspector->get_agent_info();
get_cpu_usage_and_total_procs(agent_info->start_time, cpu_usage_perc, host_cpu_usage_perc, host_procs_running);
get_rss_vsz_pss_total_memory_and_open_fds(rss, vsz, pss, host_memory_used, host_open_fds);
// Resource utilization of the agent itself
for (int i = SINSP_RESOURCE_UTILIZATION_CPU_PERC; i <= SINSP_RESOURCE_UTILIZATION_HOST_FDS; i++)
{
m_metrics.emplace_back(sinsp_stats_v2_collectors[i]());
}
}
if((m_metrics_flags & METRICS_V2_STATE_COUNTERS))
{
if (!sinsp_stats_v2)
{
m_inspector->set_sinsp_stats_v2_enabled();
sinsp_stats_v2 = m_inspector->get_sinsp_stats_v2();
}
if (sinsp_stats_v2)
{
auto thread_manager = m_inspector->m_thread_manager.get();
if (thread_manager)
{
n_threads = thread_manager->get_thread_count();
threadinfo_map_t* threadtable = thread_manager->get_threads();
if (threadtable)
{
threadtable->loop([&n_fds] (sinsp_threadinfo& tinfo)
{
sinsp_fdtable* fdtable = tinfo.get_fd_table();
if (fdtable != nullptr)
{
n_fds += fdtable->size();
}
return true;
});
}
}
// Resource utilization of the agent itself
for (int i = SINSP_STATS_V2_N_THREADS; i < SINSP_MAX_STATS_V2; i++)
{
m_metrics.emplace_back(sinsp_stats_v2_collectors[i]());
}
}
}
/*
* plugins metrics
*/
if(m_metrics_flags & METRICS_V2_PLUGINS)
{
for (auto& p : m_inspector->get_plugin_manager()->plugins())
{
std::vector<metrics_v2> plugin_metrics = p->get_metrics();
m_metrics.insert(m_metrics.end(), plugin_metrics.begin(), plugin_metrics.end());
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! Thanks Melissa, completely overlooked them!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed :)

@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 17, 2024

/milestone 0.18.0

@poiana poiana added this to the 0.18.0 milestone Jun 17, 2024
@poiana poiana added size/XXL and removed size/XL labels Jun 18, 2024
@FedeDP FedeDP force-pushed the perf_improvements branch 3 times, most recently from 6bc1a8e to f2eb192 Compare June 18, 2024 10:18
@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 18, 2024

Mmmh seems like the job is hit by: actions/runner#2402

So, the job is failing on our cncf x64 self-hosted node too; but i manually tried on the node and it worked fine.
I think there is something wrong with how github actions spawn sub-process/shells (?)...

EDIT: I also tried to run perf on our self-hosted node using command specified here: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#defaultsrunshell for bash and sh.
They both work fine.

@FedeDP FedeDP force-pushed the perf_improvements branch from 2fbb50a to 768e031 Compare June 18, 2024 11:58
Copy link

Perf diff from master - unit tests

     6.53%     +1.31%  [.] sinsp::next
     6.57%     -1.14%  [.] sinsp_thread_manager::find_thread
     5.31%     -1.08%  [.] sinsp_evt::get_type
     0.95%     +0.88%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>
     1.12%     -0.48%  [.] sinsp_fdtable::find_ref
     2.15%     +0.46%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     0.70%     +0.46%  [.] libsinsp::events::is_unknown_event
     1.98%     -0.44%  [.] scap_event_decode_params
     1.07%     -0.44%  [.] sinsp_utils::find_longest_matching_evt_param
     1.05%     +0.36%  [.] sinsp_evt::get_ts

Perf diff from master - scap file

     4.93%     +6.94%  [.] sinsp_thread_manager::find_thread
    24.77%     -3.89%  [.] sinsp_filter_check::extract_nocache
    14.80%     -3.25%  [.] sinsp::next
    10.43%     -3.04%  [.] sinsp_filter_check::apply_transformers
     4.93%     +1.42%  [.] sinsp_filter_check::get_field_info
     4.93%     +1.38%  [.] gzfile_read
     5.35%     +1.32%  [.] sinsp_filter_check::extract
     4.85%     +1.13%  [.] libsinsp::runc::match_container_id
    15.16%     -0.84%  [.] sinsp_filter_check_event::extract_single
     4.93%     -0.49%  [.] sinsp_evt::get_thread_info

Heap diff from master - unit tests

total runtime: 0.03s.
calls to allocation functions: 115 (4259/s)
temporary memory allocations: 21 (777/s)
peak heap memory consumption: -160.21K
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

total runtime: -0.01s.
calls to allocation functions: 0 (0/s)
temporary memory allocations: -5 (500/s)
peak heap memory consumption: -4.06K
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

@FedeDP FedeDP force-pushed the perf_improvements branch from 994b28c to 1b1a5a2 Compare June 25, 2024 10:56
leogr
leogr previously approved these changes Jun 25, 2024
Copy link

Perf diff from master - unit tests

     6.58%     -1.00%  [.] sinsp_thread_manager::find_thread
     6.54%     +0.89%  [.] sinsp::next
    10.89%     -0.81%  [.] sinsp_parser::reset
     1.36%     +0.80%  [.] sinsp::fetch_next_event
     3.84%     -0.75%  [.] sinsp_parser::process_event
     4.77%     -0.53%  [.] next
     2.15%     +0.49%  [.] std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release
     0.95%     +0.47%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>
     0.15%     +0.47%  [.] sinsp_evt::get_num_params
     2.08%     -0.38%  [.] libsinsp::sinsp_suppress::process_event

Perf diff from master - scap file

    22.48%     -5.31%  [.] sinsp_filter_check::extract_nocache
     4.40%     +5.07%  [.] libsinsp::runc::match_container_id
     9.47%     +2.75%  [.] sinsp_filter_check::apply_transformers
     4.47%     +2.67%  [.] sinsp_filter_check::rawval_to_string
     8.95%     -2.11%  [.] sinsp_filter_check::get_transformed_field_info
     4.47%     +1.51%  [.] sinsp_filter_check::get_field_info
     4.48%     +1.48%  [.] sinsp_threadinfo::~sinsp_threadinfo
     4.47%     -1.04%  [.] sinsp_evt::get_thread_info
     4.48%     -1.03%  [.] sinsp_thread_manager::find_thread
     4.48%     -1.03%  [.] gmt2local

Heap diff from master - unit tests

total runtime: 0.03s.
calls to allocation functions: 532 (17161/s)
temporary memory allocations: 261 (8419/s)
peak heap memory consumption: -160.21K
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

total runtime: -0.01s.
calls to allocation functions: 0 (0/s)
temporary memory allocations: -5 (454/s)
peak heap memory consumption: -4.06K
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

@FedeDP FedeDP changed the title chore(userspace/libsinsp): perf improvements chore(userspace/libsinsp): keep a reference on m_sinsp_stats_v2 where needed Jun 25, 2024
Andreagit97
Andreagit97 previously approved these changes Jun 25, 2024
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 25, 2024

/hold

FedeDP added 5 commits June 25, 2024 13:13
Properly keep a reference on m_sinsp_stats_v2 where needed, instead of fetching it every time.
Moreover, improve perf in `sinsp_utils::ts_to_string`: cache `gmt2local` result instead of fetching it every time as it is an heavy operation.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
… return empty value, to be skipped, when requirements are not met.

For now, this means that metrics that require `m_sinsp_stats_v2` will be automatically skipped when it is disabled.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
…ifact action.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP FedeDP dismissed stale reviews from Andreagit97 and leogr via 377edd5 June 25, 2024 11:14
@FedeDP FedeDP force-pushed the perf_improvements branch from 1b1a5a2 to 377edd5 Compare June 25, 2024 11:14
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@poiana
Copy link
Contributor

poiana commented Jun 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [Andreagit97,FedeDP,leogr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

Perf diff from master - unit tests

     0.81%     +1.02%  [.] sinsp_parser::event_cleanup
     6.58%     -0.88%  [.] sinsp_thread_manager::find_thread
     5.32%     -0.84%  [.] sinsp_evt::get_type
     1.12%     -0.71%  [.] sinsp_fdtable::find_ref
     1.36%     +0.60%  [.] sinsp::fetch_next_event
     0.91%     -0.57%  [.] scap_event_has_large_payload
     4.77%     +0.56%  [.] next
     3.80%     -0.50%  [.] sinsp_evt::load_params
     1.07%     +0.46%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
     1.36%     +0.46%  [.] std::_Hashtable<long, std::pair<long const, std::shared_ptr<sinsp_threadinfo> >, std::allocator<std::pair<long const, std::shared_ptr<sinsp_threadinfo> > >, std::__detail::_Select1st, std::equal_to<long>, std::hash<long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_before_node

Perf diff from master - scap file

     5.21%     +3.24%  [.] sinsp_filter_check::rawval_to_string
    11.01%     -3.02%  [.] sinsp_filter_check::apply_transformers
    10.41%     -2.75%  [.] sinsp_filter_check::get_transformed_field_info
    16.00%     -1.50%  [.] sinsp_filter_check_event::extract_single
     5.12%     -0.56%  [.] libsinsp::runc::match_container_id
     5.45%     -0.53%  [.] main
    26.16%     -0.53%  [.] sinsp_filter_check::extract_nocache
     5.21%     -0.50%  [.] sinsp_parser::process_event
     5.21%     -0.44%  [.] binary_buffer_to_string_dots
     5.20%     -0.42%  [.] sinsp_filter_check::get_field_info

Heap diff from master - unit tests

total runtime: 0.03s.
calls to allocation functions: 225 (7031/s)
temporary memory allocations: 80 (2500/s)
peak heap memory consumption: -160.21K
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

total runtime: 0.00s.
calls to allocation functions: 0 (0/s)
temporary memory allocations: -2 (-1000/s)
peak heap memory consumption: -4.06K
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 25, 2024

/unhold

@poiana poiana merged commit 473bf06 into master Jun 25, 2024
41 checks passed
@poiana poiana deleted the perf_improvements branch June 25, 2024 11:46
@LucaGuerra LucaGuerra mentioned this pull request Jul 23, 2024
@FedeDP
Copy link
Contributor Author

FedeDP commented Jul 31, 2024

/milestone 0.17.3

@poiana poiana modified the milestones: 0.18.0, 0.17.3 Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants