update to use new memory_tools from osrf_testing_tools_cpp by wjwwood · Pull Request #101 · ros2/rcutils

wjwwood · 2018-05-01T10:28:08Z

This is just a pr to use the new memory tools that I refactored out of this package and rcl too. After this will come a pr to make use of the memory tools in this repository more and fix memory allocation issues.

wjwwood · 2018-05-01T10:30:14Z

CMakeLists.txt

+    '-D', 'rcutils_module_path=\"${rcutils_module_path}\"', \
+    '${CMAKE_CURRENT_SOURCE_DIR}/resource/logging_macros.h.em' \
+  ] \
+)")


@dhood I broke this line up because it was pretty long. It appears to work correctly, but if you'd prefer it not to be this way I can revert it.

fine by me thanks

wjwwood · 2018-05-01T10:33:12Z

First round of CI:

Linux
Linux-aarch64
macOS
Windows

dejanpan · 2018-05-07T23:34:56Z

@serge-nikulin fyi

serge-nikulin · 2018-05-07T23:52:20Z

yes, I'm a subscriber.

wjwwood · 2018-05-08T19:21:05Z

Updated CI without git-lfs being used in osrf_testing_tools_cpp:

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2018-05-22T04:22:32Z

test/test_logging_macros.c

    return 5;
  }
-  if (g_last_log_event.location->line_number != 65u) {
+  if (g_last_log_event.location->line_number != line_number) {


@dhood FYI I changed this too, does that seem ok to you?

wjwwood · 2018-05-22T04:28:12Z

New CI:

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2018-06-04T19:54:07Z

New CI with fixes for previous failures:

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2018-06-05T15:53:49Z

Another round of CI (fixed the macOS issue, but aarch64 is still failing in a strange way):

Linux
Linux-aarch64
macOS
Windows

wjwwood · 2018-06-05T20:48:53Z

I'm working on the issue on aarch64.

dhood · 2018-06-05T21:37:30Z

these PRs are back in review right @wjwwood ? (other than the arm issue)

wjwwood · 2018-06-05T23:55:22Z

Feel free to review but I’m still debugging the aarch64 issue.

wjwwood · 2018-06-07T01:01:33Z

I've spent enough time on the aarch64 issue that I'm not convinced I can sort it out in time for the release. Also, I'm convinced that it's only on shutdown (memory tools is working otherwise). So I've made an issue, collecting my debug information:

osrf/osrf_testing_tools_cpp#3

And I'm going to disable memory tools on ARM for now. I'll put this in review again after I update our cmake to avoid doing memory tools on ARM, in addition to already avoiding it on Windows.

serge-nikulin · 2018-06-07T01:07:29Z

@dejanpan, FYI

dejanpan · 2018-06-07T07:21:42Z

@wjwwood ack. It is what it is. Review again tmr?

dhood

overall looks good but I have some questions before approving

dhood · 2018-06-11T21:13:41Z

test/test_allocator.cpp

 */
 TEST_F(CLASSNAME(TestAllocatorFixture, RMW_IMPLEMENTATION), test_default_allocator_normal) {
-  ASSERT_NO_MALLOC(
+  EXPECT_NO_MALLOC(


is there a default callback? if not, I think that since the callbacks have been removed above, this won't do anything if an unexpected malloc occurs? Maybe it should be wrapped in something like this?

Looks like a bug. I should setup the callbacks before that.

dhood · 2018-06-11T21:17:19Z

test/test_time.cpp

+    on_unexpected_calloc([]() {FAIL() << "UNEXPECTED CALLOC";});
+    on_unexpected_free([]() {FAIL() << "UNEXPECTED FREE";});
+    enable_monitoring_in_all_threads();
+    EXPECT_NO_MEMORY_OPERATIONS(;);


could you add a comment for what this is trying to ensure?

Not sure, looks like a copy-paste typo. I'll remove it.

dhood · 2018-06-11T21:23:00Z

test/test_time.cpp

+using osrf_testing_tools_cpp::memory_tools::on_unexpected_calloc;
+using osrf_testing_tools_cpp::memory_tools::on_unexpected_free;
+using osrf_testing_tools_cpp::memory_tools::on_unexpected_malloc;
+using osrf_testing_tools_cpp::memory_tools::on_unexpected_realloc;


it's a bit confusing to not see any usage of these in this file (I know they come from the EXPECT_NO_MEMORY_OPERATIONS macro).. I imagine there's a reason they either can't or shouldn't be embedded in the macro itself, but wanted to point out my concern (from a usability perspective) nonetheless

I'll see if I can clean that up.

dhood · 2018-06-11T21:28:51Z

test/test_time.cpp

-    set_on_unexpected_free_callback([]() {ASSERT_FALSE(true) << "UNEXPECTED FREE";});
-    start_memory_checking();
+    osrf_testing_tools_cpp::memory_tools::initialize();
+    on_unexpected_malloc([]() {FAIL() << "UNEXPECTED MALLOC";});


I know you probably had a reason for favouring the new name, but I did want to point out from a user perspective that set_*_callback seemed to better capture what this function was doing. Given the other macros e.g. EXPECT_NO_MEMORY_OPERATIONS taking in a set of statements to check, now when I look at something called on_unexpected_malloc I expect it to similarly be checking for unexpected malloc on a particular set of statements (which is not what it's doing). We can survive with the naming either way though, for sure.

I was following the on_<event>(callback) style of things like JQuery, e.g. https://api.jquery.com/on/. Personally I find the new syntax easier to read, but that's subjective. I don't really follow:

I expect it to similarly be checking for unexpected malloc on a particular set of statements (which is not what it's doing).

Reading that, it's like on_unexpected_malloc(<statements>) becomes "on an unexpected malloc for statements <statements> do...?". It's an incomplete thought.

So I agree that set_*_callback is very unambiguous, but I don't exactly see the issue with on_*, and it's 9 characters shorter.

My personal preference would be to keep the new name, but I'm open to other arguments.

Yeah, I don't mean that the function as it's named is exceptionally misleading, just that on first glance I expected that line to be doing checking for unexpected mallocs as opposed to setting a callback, whereas with the other naming it was clearer. If that's a naming pattern used by other languages e.g. jquery as you mentioned, then that's fine now that it's been changed

on first glance I expected that line to be doing checking for unexpected mallocs as opposed to setting a callback

That's what I was talking about too, but the keyword on doesn't make sense to me without a callback, as-in on <something> do <this (callback)>, so I could imagine on taking statements and a callback, but not just statements. So maybe it's important to understand why on indicates to you (and potentially users) that something is being checked right there.

At any rate, it's documented:

https://github.com/osrf/osrf_testing_tools_cpp/blob/67b321750a863cd0e2a49a8ddd5b1227031513de/osrf_testing_tools_cpp/include/osrf_testing_tools_cpp/memory_tools/testing_helpers.hpp#L26

wjwwood · 2018-06-11T23:09:22Z

I addressed the comments (I think, please have another look @dhood), here's new CI with --packages-up-to test_osrf_testing_tools_cpp rcutils rcl:

Linux
Linux-aarch64
macOS
Windows

dhood · 2018-06-11T23:37:25Z

as a note: I haven't reviewed the new code in the new repo

wjwwood · 2018-06-11T23:40:40Z

@dhood that's fine. I think reviewing its usage here and having the tests pass will be good enough for now. Future changes can be reviewed in more detail.

wjwwood added enhancement New feature or request in review Waiting for review (Kanban column) labels May 1, 2018

wjwwood self-assigned this May 1, 2018

wjwwood mentioned this pull request May 1, 2018

add osrf_testing_tools_cpp ros2/ros2#488

Merged

wjwwood commented May 1, 2018

View reviewed changes

wjwwood added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels May 3, 2018

wjwwood mentioned this pull request May 11, 2018

Dynamic memory audit #102

Merged

wjwwood added 4 commits May 21, 2018 21:20

remove old memory tools

a39e381

use new memory tools from osrf_testing_tools_cpp

ca48992

[style] make lines shorter

dd14240

change logging macro tests to not need to be manually update anymore

a823720

wjwwood force-pushed the use_osrf_testing_tools_cpp branch from 149e556 to a823720 Compare May 22, 2018 04:21

wjwwood commented May 22, 2018

View reviewed changes

wjwwood mentioned this pull request May 22, 2018

update to use new memory_tools from osrf_testing_tools_cpp ros2/rcl#238

Merged

dhood reviewed Jun 11, 2018

View reviewed changes

skip allocator test on aarch64 too, for now

547f763

address comments

1d10bec

dhood approved these changes Jun 11, 2018

View reviewed changes

fix skipping logic

727189e

wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jun 12, 2018

wjwwood merged commit 1f9cfc5 into master Jun 12, 2018

wjwwood deleted the use_osrf_testing_tools_cpp branch June 12, 2018 07:13

rohbotics removed the in review Waiting for review (Kanban column) label Jun 12, 2018

Conversation

wjwwood commented May 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood commented May 1, 2018

Uh oh!

dejanpan commented May 7, 2018

Uh oh!

serge-nikulin commented May 7, 2018

Uh oh!

wjwwood commented May 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wjwwood commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjwwood commented Jun 4, 2018

Uh oh!

wjwwood commented Jun 5, 2018

Uh oh!

wjwwood commented Jun 5, 2018

Uh oh!

dhood commented Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjwwood commented Jun 5, 2018

Uh oh!

wjwwood commented Jun 7, 2018

Uh oh!

serge-nikulin commented Jun 7, 2018

Uh oh!

dejanpan commented Jun 7, 2018

Uh oh!

dhood 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!

wjwwood commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhood commented Jun 11, 2018

Uh oh!

wjwwood commented Jun 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wjwwood commented May 22, 2018 •

edited

Loading

dhood commented Jun 5, 2018 •

edited

Loading

wjwwood commented Jun 11, 2018 •

edited

Loading