Skip to content

stats: Include a full log of stats memory changes in the memory test, and employ exact comparisons #6688

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.

Sign up for GitHub

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

Merged
htuch merged 1 commit into from
Apr 24, 2019
Merged

stats: Include a full log of stats memory changes in the memory test, and employ exact comparisons #6688

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions test/integration/stats_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,22 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithStats) {

EXPECT_LT(start_mem, m1);
EXPECT_LT(start_mem, m1001);
// As of 2019/04/12, m_per_cluster = 59576 (libstdc++)
EXPECT_LT(m_per_cluster, 59600);

// Note: if you are increasing this golden value because you are adding a
// stat, please confirm that this will be generally useful to most Envoy
// users. Otherwise you are adding to the per-cluster memory overhead, which
// will be significant for Envoy installations that are massively
// multi-tenant.
//
// History of golden values:
//
// Date PR Bytes Per Cluster Notes
// ---------- ----- ----------------- -----
// 2019/03/20 6329 59015 Initial version
// 2019/04/12 6477 59576 Implementing Endpoint lease...
// 2019/04/23 6659 59512 Reintroduce dispatcher stats...

EXPECT_EQ(m_per_cluster, 59512);
Copy link
Member

@htuch htuch Apr 24, 2019

Choose a reason for hiding this comment

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

Do we only run this on one particular build type and compiler? Just wondering how we can be so precise. Looks good otherwise.

Copy link
Contributor Author

@jmarantz jmarantz Apr 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Well the compiler doesn't appear to matter that much, at least between g++ and clang. We do run only in one STL impl -- at one point in @cmluciano 's PR we were trying to work with two different libraries but abandoned that for reasons I forgot.

If any stability issues arise on any particular platform, we should adjust hasDeterministicMallocStats to skip that one.

Copy link
Member

@htuch htuch Apr 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Don't we use libc++ on OS X?

Copy link
Contributor Author

@jmarantz jmarantz Apr 24, 2019

Choose a reason for hiding this comment

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

Maybe we use that, but then either: hasDeterministicMallocStats() returns false for some other reason (e.g. not using TCMALLOC) or libc++ has exactly the same memory usage for the operations this is measuring, as CI seems to work on Mac.

Copy link
Member

@htuch htuch Apr 24, 2019

Choose a reason for hiding this comment

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

I'll merge, but I think this whole situation is very fragile and will probably break new architectures/compilers. I hope we aren't using hasDeterministicMallocStats() for any compiler or STL with a different layout than the one we expect.

Copy link
Contributor Author

@jmarantz jmarantz Apr 24, 2019

Choose a reason for hiding this comment

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

Thanks Harvey -- I think it will be a pretty clear test failure if that happens that could be addressed by changing the definition of hasDeterministicMallocStats() (and maybe its name...maybe 'deterministic' is the wrong name for that....)

In the meantime, I've noticed since starting on this journey of trying to prune memory that it's bloated quite a bit and so I'm hoping this makes us more conscious of costs.

}

} // namespace
Expand Down