Page MenuHomePhabricator

[libc] revamp memory function benchmark
ClosedPublic

Authored by gchatelet on Dec 14 2020, 6:41 AM.

Details

Summary

The benchmarking infrastructure can now run in two modes:

  • Sweep Mode: which generates a ramp of size values (same as before),
  • Distribution Mode: allows the user to select a distribution for the size paramater that is representative from production.

The analysis tool has also been updated to handle both modes.

Diff Detail

Event Timeline

gchatelet created this revision.Dec 14 2020, 6:41 AM
Herald added a project: Restricted Project. ยท View Herald TranscriptDec 14 2020, 6:41 AM
gchatelet requested review of this revision.Dec 14 2020, 6:41 AM
courbet added inline comments.Dec 16 2020, 12:51 AM
libc/benchmarks/MemorySizeDistributions.cpp
43

Would it make sense to statically generate the uniform array and just get rid of all the code to handle uniform gen ? We could remove a lot of code with the cost of one additional array here.

gchatelet updated this revision to Diff 312159.Dec 16 2020, 2:07 AM
  • Address comments and fix naming style
gchatelet marked an inline comment as done.Dec 16 2020, 2:07 AM
courbet accepted this revision.Dec 17 2020, 4:42 AM

Only cosmetic comments.

libc/benchmarks/LibcMemoryBenchmark.h
41

Make this a method ?

bool IsSweepMode() {
  return SweepModeMaxSize > 0;
}
libc/benchmarks/LibcMemoryBenchmarkMain.cpp
74

OffsetBytes ?

75

SizeBytes ?

106

did you mean & ?

149

why not const & ?

libc/benchmarks/README.md
98

I think you can drop this.

This revision is now accepted and ready to land.Dec 17 2020, 4:42 AM
gchatelet updated this revision to Diff 312452.Dec 17 2020, 5:04 AM
gchatelet marked 6 inline comments as done.
  • Address comments
gchatelet updated this revision to Diff 312453.Dec 17 2020, 5:05 AM

rebasing

libc/benchmarks/LibcMemoryBenchmark.h
41

I'm not happy with the redundant information either.
That said the structure is shared between C++ and the external world via JSON and I found it simpler to not replicate the logic everywhere and have a simple boolean attribute.

libc/benchmarks/LibcMemoryBenchmarkMain.cpp
106

Both work but & is clearer. Thx for the suggestion.

149

Good catch!

@sivachandra I'll submit this as is. Let me know if you have any concerns and we can amend it in a follow up patch.

This revision was landed with ongoing or failed builds.Dec 17 2020, 5:24 AM
This revision was automatically updated to reflect the committed changes.

@sivachandra I'll submit this as is. Let me know if you have any concerns and we can amend it in a follow up patch.

Sorry, I am no expert here so I just kept silent while @courbet was steering the review. LGTM.

@sivachandra I'll submit this as is. Let me know if you have any concerns and we can amend it in a follow up patch.

Sorry, I am no expert here so I just kept silent while @courbet was steering the review. LGTM.

Thx ๐Ÿ‘ Next time I'll move you as subscriber instead of reviewer.