[core] Log Memory.Footprint test results #8170

anandthakker · 2017-02-22T17:48:14Z

Even when we're passing this test, it can be very useful to have a log of the memory usage value, so that we can get a sense of the memory impact of new features and also to make sure that it's reasonable when we to increase the thresholds for these checks, when necessary.

Closes #8149 (leaving integration w/ the fancy nightly metrics graph for another day)

mention-bot · 2017-02-22T17:48:16Z

@anandthakker, thanks for your PR! By analyzing this pull request, we identified @tmpsantos and @jfirebaugh to be potential reviewers.

jfirebaugh

👍 Just one suggestion for output:

Vector: 6.74865e+07 OK
Raster: 2.43709e+07 OK

Can you change these to print in regular notation (or MB units)?

tmpsantos · 2017-02-22T21:50:49Z

Can we hold on merging this? I would like to review it tomorrow with more time.

tmpsantos

I don't object to this change but looking carefully I don't see what value we are adding moving it from gtest to gbenchmark.

gbenchmark was designed to see how many times you can run a test in a time frame and I would prefer to keep it this way.

For instance the json output of gbenchmark with this test would be misleading.

Why not keep it on gtest? I think for this particular case it is even more prepared for having a XML generated that we could use to plot a graph:

https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#logging-additional-information

tmpsantos · 2017-02-23T12:02:38Z

benchmark/include/mbgl/benchmark.hpp

@@ -4,4 +4,5 @@ namespace mbgl {

 int runBenchmark(int argc, char* argv[]);

+


Not needed.

tmpsantos · 2017-02-23T12:07:32Z

benchmark/memory/memory.benchmark.cpp

+#include <unistd.h>
+
+/*
+ * Begin getRSS.c


Can we move this to benchmark/src/mbgl/benchmark/getrss.c? Maybe add the function prototypes to utils.hpp.

tmpsantos · 2017-02-23T12:10:26Z

benchmark/include/mbgl/memory.benchmark.hpp

+
+namespace mbgl {
+namespace benchmark {
+    bool memoryFootprintBenchmark();


nit, we don't indent inside the namespace

tmpsantos · 2017-02-23T12:10:49Z

.travis.yml

@@ -173,7 +173,8 @@ matrix:
        - mapbox_export_mesa_library_path
      script:
        - make qt-app
-        - LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so make run-qt-test-Memory.*:*.Load
+        - LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so make run-qt-test-*.Load


Let's keep the other memory tests.

tmpsantos · 2017-02-23T12:16:42Z

benchmark/memory/memory.benchmark.cpp

+            result.data = it->second;
+        } else {
+            auto data = std::make_shared<std::string>(
+                util::read_file(std::string("test/fixtures/resources/") + path));


Copy the test resources used by this test to benchmark/fixtures/.

tmpsantos · 2017-02-23T12:17:09Z

benchmark/memory/memory.benchmark.cpp

+/** End getRSS.c */
+
+
+using namespace mbgl;


No needed, you are already using namespace mbgl {

tmpsantos · 2017-02-23T12:22:13Z

test/util/memory.test.cpp

-    std::unordered_map<std::string, std::shared_ptr<std::string>> cache;
-};
-
-TEST(Memory, Vector) {


Do not remove this. These (Memory.Vector and Memory.Raster) are the only tests we have that do a full pass rendering inside valgrind looking for memory leaks.

anandthakker · 2017-02-23T12:46:20Z

Thanks for the review @tmpsantos

Why not keep it on gtest? I think for this particular case it is even more prepared for having a XML generated that we could use to plot a graph:

https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#logging-additional-information

Somehow in my attempts to get gtest to simply log additional info to the screen, I missed this. You're right: if we can use gtest's existing functionality, then I should rework this PR to:

Keep the more portable version of getRSS (it's convenient to be able to at least run the memory test on non-linux box, even if it's not as reliable without jemalloc)
Use gtest's RecordProperty feature to record the memory metrics to XML output
Add a simple travis step that greps (or whatever) for these values from the XML results, so that we can see them in the travis logs. Later, when we have a chance, we can add this to the nightly metrics charts.

tmpsantos · 2017-02-23T12:53:03Z

Keep the more portable version of getRSS (it's convenient to be able to at least run the memory test on non-linux box, even if it's not as reliable without jemalloc)

👍

Use gtest's RecordProperty feature to record the memory metrics to XML output
Add a simple travis step that greps (or whatever) for these values from the XML results, so that we can see them in the travis logs. Later, when we have a chance, we can add this to the nightly metrics charts.

This would be an amazing follow-up. There was prior work done here for binary size:
#7005

Metrics can be seen here:
https://mapbox.github.io/mapbox-gl-native/metrics/binary-size/

anandthakker · 2017-02-27T17:16:32Z

@tmpsantos updated to output results from gtest as we discussed, and laid the groundwork for reporting / visualizing the benchmark over time in log_memory_benchmarks.cpp

It appears there isn't currently a nightly build for qt, and I think posting for every build would be overkill. Should we add a nightly-release workflow to the qt bitrise setup?

anandthakker changed the title ~~Move Memory.Footprint from unit test => benchmark~~ [core] Move Memory.Footprint from unit test => benchmark Feb 22, 2017

anandthakker requested review from jfirebaugh and tmpsantos February 22, 2017 18:46

jfirebaugh approved these changes Feb 22, 2017

View reviewed changes

tmpsantos suggested changes Feb 23, 2017

View reviewed changes

anandthakker force-pushed the log-memory-usage branch from c20f8d4 to b9fac17 Compare February 27, 2017 16:19

Log Memory.Footprint test output

c9ae8b6

anandthakker force-pushed the log-memory-usage branch from b9fac17 to c9ae8b6 Compare February 27, 2017 17:03

anandthakker changed the title ~~[core] Move Memory.Footprint from unit test => benchmark~~ [core] Log Memory.Footprint test results Feb 27, 2017

On CI, only run Memory.Footprint test with libjemalloc

8230e44

tmpsantos approved these changes Feb 28, 2017

View reviewed changes

anandthakker merged commit 4b81c92 into master Feb 28, 2017

anandthakker deleted the log-memory-usage branch February 28, 2017 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Log Memory.Footprint test results #8170

[core] Log Memory.Footprint test results #8170

anandthakker commented Feb 22, 2017 •

edited

Loading

mention-bot commented Feb 22, 2017

jfirebaugh left a comment

tmpsantos commented Feb 22, 2017

tmpsantos left a comment

tmpsantos Feb 23, 2017

tmpsantos Feb 23, 2017

tmpsantos Feb 23, 2017

tmpsantos Feb 23, 2017

tmpsantos Feb 23, 2017

tmpsantos Feb 23, 2017

tmpsantos Feb 23, 2017

anandthakker commented Feb 23, 2017

tmpsantos commented Feb 23, 2017

anandthakker commented Feb 27, 2017

		@@ -4,4 +4,5 @@ namespace mbgl {

		int runBenchmark(int argc, char* argv[]);

[core] Log Memory.Footprint test results #8170

[core] Log Memory.Footprint test results #8170

Conversation

anandthakker commented Feb 22, 2017 • edited Loading

mention-bot commented Feb 22, 2017

jfirebaugh left a comment

Choose a reason for hiding this comment

tmpsantos commented Feb 22, 2017

tmpsantos left a comment

Choose a reason for hiding this comment

tmpsantos Feb 23, 2017

Choose a reason for hiding this comment

tmpsantos Feb 23, 2017

Choose a reason for hiding this comment

tmpsantos Feb 23, 2017

Choose a reason for hiding this comment

tmpsantos Feb 23, 2017

Choose a reason for hiding this comment

tmpsantos Feb 23, 2017

Choose a reason for hiding this comment

tmpsantos Feb 23, 2017

Choose a reason for hiding this comment

tmpsantos Feb 23, 2017

Choose a reason for hiding this comment

anandthakker commented Feb 23, 2017

tmpsantos commented Feb 23, 2017

anandthakker commented Feb 27, 2017

anandthakker commented Feb 22, 2017 •

edited

Loading