ARROW-15062: [C++] Add memory information to current spans #12702

AlvinJ15 · 2022-03-24T07:57:08Z

Add memory information to the current spans, bytes in memory_pool and RSS

github-actions · 2022-03-24T07:57:27Z

https://issues.apache.org/jira/browse/ARROW-15062

github-actions · 2022-03-24T07:57:29Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

pitrou

Some comments below.

pitrou · 2022-03-24T08:52:55Z

cpp/src/arrow/util/tracing_internal.h

Please return int64_t instead.

pitrou · 2022-03-24T08:54:08Z

cpp/src/arrow/util/tracing_internal.cc

This is unfortunately Linux-specific, we'll need a portable implementation (is there a third-party library that we can use to make our life easier?).

There was a portable implementation as part of a PR here: https://github.com/apache/arrow/pull/11426/files#diff-5d7d9a549780da2ec4baba08a43db3e1d524b70d6e8ae4f29cc8bfe831357f9cR178-R199

We didn't end up pursuing the PR but you might be able to borrow some from that particular function.

That being said, I wonder if it would be more valuable to report default_memory_pool()->bytes_allocated(). sysinfo is going to give us statistics on the entire server and freeram is sometimes misleading (e.g. if there are a lot of dirty pages then freeram could be high while at the same time we might be thrashing swap).

@westonpace I get the bytes from the memory pool in the first span, but in the next ones I extracted the statistics from the sysinfo for report the RSS, I looked the PR which you mentioned, and there the method return just the FreeRam and for calculate the Current Ram in used I still need to use sysinfo.total_memory, but after that PR get merged I can add another function for get that, and don't use system includes here.

Ah, yes, I see now that you are grabbing all three numbers. GetMemoryUsed is less interesting than GetMemoryUsedByProcess in my opinion but I don't think it would hurt to report it.

but after that PR get merged I can add another function for get that, and don't use system includes here.

That PR that I linked to will not be merged. We ended up pursuing a different approach. So we don't want to wait on it. However, you're welcome to copy the approach that was being taken for this method.

pitrou · 2022-03-24T08:54:54Z

cpp/src/arrow/util/tracing_internal.cc

Are you sure it's not 1024, rather?

pitrou · 2022-03-24T08:55:28Z

cpp/src/arrow/util/tracing_internal.cc

You'll need to check for errors here; 2) would it be easier to use std::fstream?

Also, this is probably Linux-specific...

pitrou · 2022-03-24T08:57:37Z

cpp/src/arrow/util/tracing_internal.cc

We try to write C++ code not C code, so this function should take a util::string_view IMHO.

pitrou · 2022-03-24T08:58:13Z

cpp/src/arrow/util/tracing_internal.cc

Which one? The one returned by the function?

The value obtained from the system. This function was re-implemented and documented in the io_utils.h

pitrou · 2022-03-24T08:58:42Z

cpp/src/arrow/util/tracing_internal.h

These functions should go into arrow/util/io_util.{h,cc} instead.

pitrou · 2022-03-24T08:59:50Z

cpp/src/arrow/compute/exec/aggregate_node.cc

These calls are potentially costly since they will be issueing system calls (and perhaps even read a file). I'm not sure this is desirable. @lidavidm What do you think?

Yes, this seems it will be quite expensive. Do we need information that's this fine-grained?

We could cache the values, and only update them if they're out of date. Or we could actually record the memory pool's statistics instead. Also, this information isn't really thread- or span- specific. Maybe it could be done by a background thread on a fixed schedule, especially if it's process memory and not memory pool allocations?

Background thread on a fixed schedule was the original ask but I think there was some concern around having two streams of telemetry information. As a start I think only using memory_pool_bytes would be sufficient. Longer term it could be handy to have some idea of the RSS of the process just to have some sense for how we are doing on fragmentation and non-pool allocations. Since a system call is expensive I wonder if there would be a way to debounce this information and only report it at most every 10ms or on some configurable threshold.

@westonpace I can create the background thread for report the system RAM, but where can I place it? inside the memory pool constructor? and using the same exporter or creating a new one just for this.

To be clear, I am not asking for a background thread in that comment. It is ok either way. I have no preference either way. I only mentioned the background thread in the comment to explain the context.

In both cases (background thread or span processor) I now think we want to move the initialization into the query testing tool (which unfortunately, is not yet merged). This means the feature will not be enabled for library users but I think that is ok.

pitrou · 2022-03-24T09:01:07Z

cpp/src/arrow/compute/exec/aggregate_node.cc

We should certainly avoid repeating this everywhere, and instead find a way to factor out those common span attributes (perhaps using a dedicated macro?).

This could be done by a span processor, though that has a couple caveats:

Processors are configured as part of the tracing setup, by the end user application (generally not by a library),

IIRC, processors may not run immediately (one of the processors batches spans and processes them on a background thread, I think)

I think some span recorders can also be configured to attach this sort of information as well? (Though again, the timestamps won't line up anymore.)

Given that these memory statistics are process-wide anyways maybe it makes sense to only enable collection of memory statistics in our own profiling tools. For example, when running the query tester we can add a span processor that collects memory statistics.

Then the actual library code would only instrument things specific to the library.

That only leaves the batching concern but, glancing at the description, and knowing nothing of the OT spec, I think the span processor itself might batch its output but its input (the span start/stop calls) won't be batched.

westonpace

Thank you for writing detailed methods for fetching the RSS. That will be very useful. It doesn't look like we are using that yet. Perhaps the plan is to wait and integrate it in later into the query tester? I think it is ok but can you create a follow-up PR for the feature that would use this?

Also, we will need test cases for GetCurrentRSS before this merges. I don't think it needs to be very complicated (e.g. we don't need to verify, in a unit test, that the RSS is exactly correct, only that it is non-zero and doesn't crash).

westonpace · 2022-04-12T23:41:04Z

cpp/src/arrow/util/io_util.cc

Nit: Can we merge these two ifdef sections?

westonpace · 2022-04-12T23:42:43Z

cpp/src/arrow/util/io_util.cc

Can we just use a dummy implementation in this case that returns 0 rather than prevent compilation? Actually, it looks like that might be what you do already so can you just remove this error?

westonpace · 2022-04-13T00:02:44Z

cpp/src/arrow/util/io_util.cc

Does this branch actually get used in the implementation?

it was unused and was removed.

westonpace · 2022-04-13T00:03:25Z

cpp/src/arrow/util/io_util.h

Suggested change

/// \brief Get the current memory used by current process in bytes

///

/// This function support Windows and Linux

/// \brief Get the current memory used by the current process in bytes

///

/// This function supports Windows, Linux, and Mac and will return 0 otherwise

westonpace · 2022-04-13T00:04:40Z

r/src/arrowExports.cpp

I'm not sure these changes are related. Maybe a rebase will get them to go away?

westonpace · 2022-04-13T00:10:31Z

cpp/src/arrow/util/io_util.cc

In general we try to avoid directly copy/pasting chunks of code from SO for licensing reasons but I don't know that we ever got an authoritative answer for this. Either way, I think things are paraphrased enough by this point it isn't a concern.

westonpace · 2022-04-13T00:13:43Z

cpp/src/arrow/util/io_util.cc

We use #if __APPLE__ quite a bit in the code base. However, we never use __MACH__. Do you know what this is adding that __APPLE__ doesn't already specify?

https://sourceforge.net/p/predef/wiki/OperatingSystems/ here it uses both conditions, but it seems to be ok if I just let
__APPLE__

westonpace · 2022-04-13T00:32:21Z

cpp/src/arrow/util/io_util.cc

I'm not sure this include is used anywhere.

westonpace · 2022-04-13T00:33:46Z

cpp/src/arrow/util/io_util.cc

fclose returns a return code. If we want to ignore it (might be ok here, not sure in what situations that close would fail and what we would even do about it) then it might be best to wrap in ARROW_UNUSED so it is explicit or add a comment.

westonpace · 2022-04-13T00:34:52Z

cpp/src/arrow/util/io_util.cc

Suggested change

return (int64_t)info.WorkingSetSize;

return static_cast<int64_t>(info.WorkingSetSize);

For consistency with the rest of the codebase here (and elsewhere in this file) we should prefer C++ style casts:

cpp/src/arrow/util/io_util.cc

pitrou · 2022-04-13T13:33:19Z

cpp/src/arrow/util/io_util.cc

Can we log the error using e.g. ARROW_LOG(WARNING)?

pitrou · 2022-04-13T13:33:29Z

cpp/src/arrow/util/io_util.cc

Same here: log the error?

pitrou · 2022-04-13T13:35:23Z

cpp/src/arrow/util/tracing_internal.cc

Two things: 1) can we use the C++ includes where available (e.g. <cstdio> instead of <stdio.h>)? 2) can we keep the system includes alphabetically ordered?

Why were these includes added to this particular file anyways? Is this a case where we weren't following IWYU and you are making it more clear?

pitrou · 2022-04-13T13:36:27Z

cpp/src/arrow/compute/exec/aggregate_node.cc

Instead of adding this by hand everywhere, can we define for example a START_COMPUTE_SPAN macro that does it automatically?

AlvinJ15 · 2022-04-20T05:36:32Z

@mbrobbel

pitrou · 2022-04-20T08:37:27Z

cpp/src/arrow/util/io_util.cc

AFAIK, __linux__ is sufficient.

pitrou · 2022-04-20T08:42:29Z

cpp/src/arrow/util/io_util.cc

Simplify this code a bit, also the ifstream destructor should close the file for you:

Suggested change

if (fp.is_open()) {

fp >> rss;

} else {

ARROW_LOG(WARNING) << "Can't resolve RSS value from /proc/self/statm";

return static_cast<int64_t>(0L);

}

if (fp.is_open()) fp.close();

return static_cast<int64_t>(rss) * static_cast<int64_t>(sysconf(_SC_PAGESIZE));

if (fp) {

fp >> rss;

return rss * sysconf(_SC_PAGESIZE);

} else {

ARROW_LOG(WARNING) << "Can't resolve RSS value from /proc/self/statm";

return 0;

}

pitrou · 2022-04-20T08:44:19Z

cpp/src/arrow/util/io_util.cc

Nit: no need for an explicit cast.

Suggested change

return static_cast<int64_t>(0L); // Unsupported.

return 0; // Unsupported.

@AlvinJ15 there's still unaddressed feedback, can you make sure everything is addressed?

@lidavidm now was solved.

pitrou · 2022-04-20T08:44:44Z

cpp/src/arrow/util/io_util.cc

Nit: no need for an explicit cast

Suggested change

return static_cast<int64_t>(0L);

return 0;

pitrou · 2022-04-20T08:47:24Z

cpp/src/arrow/util/tracing_internal.h

I'm not sure how those work in OpenTelemetry, but this seems to close the scope at the end of the macro, is this right? @lidavidm

Yes, we shouldn't create the OT scope in a C++ scope here, because the OT scope object controls how long a span is active.

@mbrobbel i put the scopes because auto opentelemetry_scope##__LINE__ when i use two START_SPAN it gives and conflict error in the variable name declaration, the opentelemetry_scope seems to be unused and when I put the scope for avoid the variable name declaration conflict seems to work fine https://paste.ofcode.org/gGUZ3Kk9H9HuyeLdJCpCX7

The scope is an RAII guard, you wouldn't generally actually use it. But effectively it makes no sense to put a scope inside a C++ block by itself.

pitrou · 2022-04-20T08:49:09Z

cpp/src/arrow/util/tracing_internal.h

I don't really understand why this starting two spans (or starting the same span twice)? @lidavidm Does this look right?

Yeah, this seems to create two spans, and immediately closes the first one. I don't think this is what we want.

Ah I see now. START_SPAN is just activating the span. I think what you want is a macro to attach a new attribute to the span, not repeating the START_SPAN macro.

I tried, to attach the new attribute to the current _VA_ARGS but there's no solution on how to remove the curly brackets {} and insert the new attribute and then pass it to the START_SPAN, I tried even create a std::vector<> insert the new attribute and convert it to std::initializer_list or create the OT::nostd::span<> manually but nothing worked

Just call span->SetAttribute. Look at the header file or docs. https://github.com/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/trace/span.h#L46-L50

lidavidm · 2022-04-20T12:17:10Z

@AlvinJ15 can you demonstrate the kind of output this gives?

AlvinJ15 · 2022-04-20T18:40:01Z

@AlvinJ15 can you demonstrate the kind of output this gives?

@lidavidm this is the output https://paste.ofcode.org/gGUZ3Kk9H9HuyeLdJCpCX7

lidavidm

Thanks. Mostly I'm just wondering if we can limit the number of macros we need.

lidavidm · 2022-04-21T00:31:49Z

cpp/src/arrow/util/tracing_internal.h

What's this define for?

lidavidm · 2022-04-21T00:32:54Z

cpp/src/arrow/util/tracing_internal.h

I don't think we need to rearrange all these macros, right? You can keep the original START_SPAN macros and just have START_COMPUTE_SPAN be START_SPAN + a call to SetAttribute. Spans are mutable, there's no requirement that you fully configure them before using them.

Mostly I don't want to have a whole bunch of macros for little things.

Or actually, maybe just have START_SPAN and CONFIGURE_COMPUTE_SPAN or something. That way we have composable macros instead of having to define duplicate macros for every scenario.

I created those macros just for don't repeat the same code twice, but yes I can delete them and copy those lines

I don't follow, these macros have repeated code right now

lidavidm · 2022-04-21T00:33:32Z

cpp/src/arrow/util/tracing_internal.h

nit, but should we perhaps namespace this attribute name? "arrow.memory_pool_bytes" or something

Note how the official ones are semi-namespaced: https://github.com/open-telemetry/opentelemetry-cpp/blob/4f5505f9cb45326299067cfbd53db21198970aa8/sdk/include/opentelemetry/sdk/resource/experimental_semantic_conventions.h

westonpace

Thanks for sticking with this. The XYZ_COMPUTE_SPAN macros will help a lot.

westonpace · 2022-04-21T07:49:16Z

cpp/src/arrow/util/io_util.cc

Suggested change

#elif defined(__linux__) || defined(__linux) || defined(linux) || defined(__gnu_linux__)

#elif defined(__linux__)

westonpace · 2022-04-21T07:49:28Z

cpp/src/arrow/util/io_util_test.cc

Suggested change

#elif defined(__linux__) || defined(__linux) || defined(linux) || defined(__gnu_linux__)

#elif defined(__linux__)

lidavidm

Looks fine to me, though if we really cared about duplicating code, we can just have

#define SETUP_COMPUTE_SPAN(span) span->SetAttribute(...);

...

START_SPAN(span);
SETUP_COMPUTE_SPAN(span);

at the very least, we should have

#define START_COMPUTE_SPAN(span, ...) \
  START_SPAN(span, __VA_ARGS__); \
  span->SetAttribute(...);

…memory_pool and RSS

… will be implemented

lidavidm · 2022-04-21T21:07:55Z

r/src/arrowExports.cpp

Did you mean to commit this?

…an atributes.

AlvinJ15 · 2022-04-28T00:54:57Z

@pitrou this could be merged?

pitrou · 2022-04-28T09:59:37Z

@AlvinJ15 Yes, definitely. Thank you for being persistent!

ursabot · 2022-05-04T11:01:41Z

Benchmark runs are scheduled for baseline = 1cd8a85 and contender = 6a7ee37. 6a7ee37 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️1.52% ⬆️0.74%] test-mac-arm
[Finished ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.75% ⬆️0.99%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 6a7ee37c ec2-t3-xlarge-us-east-2
[Finished] 6a7ee37c test-mac-arm
[Finished] 6a7ee37c ursa-i9-9960x
[Finished] 6a7ee37c ursa-thinkcentre-m75q
[Finished] 1cd8a850 ec2-t3-xlarge-us-east-2
[Finished] 1cd8a850 test-mac-arm
[Finished] 1cd8a850 ursa-i9-9960x
[Finished] 1cd8a850 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the Component: C++ label Mar 24, 2022

AlvinJ15 force-pushed the ARROW-15062#Add_regular_logging_of_exec_plan_performance_metrics branch from de15410 to c0431e0 Compare March 24, 2022 07:58

pitrou requested changes Mar 24, 2022

View reviewed changes

lidavidm changed the title ~~ARROW-15062: [C++] Add memory information to current spams, bytes in …~~ ARROW-15062: [C++] Add memory information to current spans Mar 24, 2022

AlvinJ15 force-pushed the ARROW-15062#Add_regular_logging_of_exec_plan_performance_metrics branch from c0431e0 to 2d5e1fd Compare April 6, 2022 01:15

github-actions bot added the Component: R label Apr 6, 2022

AlvinJ15 force-pushed the ARROW-15062#Add_regular_logging_of_exec_plan_performance_metrics branch 5 times, most recently from 7632150 to 002d369 Compare April 6, 2022 03:27

AlvinJ15 requested review from pitrou and westonpace April 6, 2022 03:37

AlvinJ15 force-pushed the ARROW-15062#Add_regular_logging_of_exec_plan_performance_metrics branch from 002d369 to 4cbb7fe Compare April 12, 2022 09:41

westonpace requested changes Apr 13, 2022

View reviewed changes

AlvinJ15 force-pushed the ARROW-15062#Add_regular_logging_of_exec_plan_performance_metrics branch 3 times, most recently from 7d06d7b to b6f507a Compare April 13, 2022 02:26

AlvinJ15 requested a review from westonpace April 13, 2022 02:59

pitrou reviewed Apr 13, 2022

View reviewed changes

AlvinJ15 force-pushed the ARROW-15062#Add_regular_logging_of_exec_plan_performance_metrics branch 3 times, most recently from 5c35cd1 to e85c765 Compare April 20, 2022 05:27

AlvinJ15 force-pushed the ARROW-15062#Add_regular_logging_of_exec_plan_performance_metrics branch from e85c765 to 433202a Compare April 20, 2022 05:41

AlvinJ15 requested a review from pitrou April 20, 2022 05:49

pitrou reviewed Apr 20, 2022

View reviewed changes

AlvinJ15 force-pushed the ARROW-15062#Add_regular_logging_of_exec_plan_performance_metrics branch from 433202a to dd7a099 Compare April 20, 2022 23:33

AlvinJ15 requested review from lidavidm and pitrou April 20, 2022 23:48

lidavidm reviewed Apr 21, 2022

View reviewed changes

AlvinJ15 force-pushed the ARROW-15062#Add_regular_logging_of_exec_plan_performance_metrics branch 2 times, most recently from 69ba070 to 5852ec8 Compare April 21, 2022 01:58

AlvinJ15 requested a review from lidavidm April 21, 2022 01:59

westonpace approved these changes Apr 21, 2022

View reviewed changes

lidavidm reviewed Apr 21, 2022

View reviewed changes

AlvinJ15 force-pushed the ARROW-15062#Add_regular_logging_of_exec_plan_performance_metrics branch from 5852ec8 to a88df9b Compare April 21, 2022 17:49

AlvinJ15 added 2 commits April 21, 2022 12:55

ARROW-15062: [C++] Add memory information to current spans, bytes in …

7bfecc2

…memory_pool and RSS

ARROW-15251: [C++] Remove memory system info (RSS) until query tester…

fe9a3fa

… will be implemented

AlvinJ15 force-pushed the ARROW-15062#Add_regular_logging_of_exec_plan_performance_metrics branch from a88df9b to 0ffe46c Compare April 21, 2022 18:50

AlvinJ15 requested a review from lidavidm April 21, 2022 19:42

lidavidm approved these changes Apr 21, 2022

View reviewed changes

AlvinJ15 force-pushed the ARROW-15062#Add_regular_logging_of_exec_plan_performance_metrics branch 3 times, most recently from ae06417 to 05148fa Compare April 25, 2022 02:59

AlvinJ15 requested a review from lidavidm April 25, 2022 04:05

Create START_COMPUTE_SPAN which add the memory info to the current sp…

968716a

…an atributes.

AlvinJ15 force-pushed the ARROW-15062#Add_regular_logging_of_exec_plan_performance_metrics branch from 05148fa to 968716a Compare April 25, 2022 14:39

lidavidm approved these changes Apr 25, 2022

View reviewed changes

pitrou approved these changes Apr 28, 2022

View reviewed changes

pitrou closed this in 6a7ee37 Apr 28, 2022

amoeba mentioned this pull request Mar 17, 2023

Look into adding memory info to OTel spans amoeba/arrow-flight-playground#4

Open

-/// \brief Get the current memory used by current process in bytes
-///
-/// This function support Windows and Linux
+/// \brief Get the current memory used by the current process in bytes
+///
+/// This function supports Windows, Linux, and Mac and will return 0 otherwise

	return (int64_t)info.WorkingSetSize;
	return static_cast<int64_t>(info.WorkingSetSize);

-  if (fp.is_open()) {
-    fp >> rss;
-  } else {
-    ARROW_LOG(WARNING) << "Can't resolve RSS value from /proc/self/statm";
-    return static_cast<int64_t>(0L);
-  }
-  if (fp.is_open()) fp.close();
-  return static_cast<int64_t>(rss) * static_cast<int64_t>(sysconf(_SC_PAGESIZE));
+  if (fp) {
+    fp >> rss;
+    return rss * sysconf(_SC_PAGESIZE);
+  } else {
+    ARROW_LOG(WARNING) << "Can't resolve RSS value from /proc/self/statm";
+    return 0;
+  }

	return static_cast<int64_t>(0L); // Unsupported.
	return 0; // Unsupported.

	#elif defined(__linux__) \|\| defined(__linux) \|\| defined(linux) \|\| defined(__gnu_linux__)
	#elif defined(__linux__)

ARROW-15062: [C++] Add memory information to current spans #12702

ARROW-15062: [C++] Add memory information to current spans #12702

Uh oh!

Conversation

AlvinJ15 commented Mar 24, 2022

Uh oh!

github-actions bot commented Mar 24, 2022

Uh oh!

github-actions bot commented Mar 24, 2022

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonpace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!