Skip to content
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

Fix msvc build errors in block_cache_trace_analyzer.cc (#5746) #5838

Closed
wants to merge 3 commits into from
+15 −7
Closed

Fix msvc build errors in block_cache_trace_analyzer.cc (#5746) #5838

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6974ab0
Fix msvc build errors in block_cache_trace_analyzer.cc (#5746)
Sep 20, 2019
2b6a94c
Add missing #include to fix xcode build (#5746)
Sep 22, 2019
07c8f96
Remove use of std::make_unique - xcode seems not to like it (#5746)
Sep 23, 2019
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
22 changes: 15 additions & 7 deletions tools/block_cache_analyzer/block_cache_trace_analyzer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <fstream>
#include <iomanip>
#include <iostream>
#include <memory>
Copy link
Contributor

Choose a reason for hiding this comment

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

<memory> is already included by including other headers.

Copy link
Author

Choose a reason for hiding this comment

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

You're right, I originally thought that was why the xcode build was failing. But it turns out xcode clang doesn't like std::make_unique at all, at least with the compiler options that are being used here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we can use #if defined() to enclose this include?

Copy link
Author

Choose a reason for hiding this comment

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

I would just remove it altogether. If I had looked more closely I would have realized that std::unique_ptr is already used in several other places in this file, so including is clearly not necessary for this new use of it.
Alternatively you could keep it in; some people would argue that each file should explicitly include the headers for what it uses so that it is not dependent on other header files including them first. All of those headers will have include guards so the impact on compile time should be negligible.

#include <random>
#include <sstream>

Expand Down Expand Up @@ -650,7 +651,8 @@ void BlockCacheTraceAnalyzer::WriteCorrelationFeaturesToFile(
const std::map<std::string, Features>& label_features,
const std::map<std::string, Predictions>& label_predictions,
uint32_t max_number_of_values) const {
std::default_random_engine rand_engine(env_->NowMicros());
std::default_random_engine rand_engine(
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation. Can you build_tools/format-diff.sh as described in https://github.com/facebook/rocksdb/wiki/RocksDB-Contribution-Guide?

static_cast<unsigned int>(env_->NowMicros()));
for (auto const& label_feature_vectors : label_features) {
const Features& past = label_feature_vectors.second;
auto it = label_predictions.find(label_feature_vectors.first);
Expand Down Expand Up @@ -1209,11 +1211,14 @@ void BlockCacheTraceAnalyzer::WriteBlockReuseTimeline(
TraverseBlocks(block_callback);

// A cell is the number of blocks accessed in a reuse window.
uint64_t reuse_table[reuse_vector_size][reuse_vector_size];
const std::unique_ptr<uint64_t[]> reuse_table(
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, MSVC forbids two-dimensional arrays now? Can @hliu18 or @burtonli verify this behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

MSVC supports two-dimensional arrays, but does not support variable length arrays, i.e. dimension sizes need to be known at compile time. Variable length array is not part of recent versions of C++ standard.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will the following compile and run on MSVC?

#include <cstdio>

int main(int argc, char** argv) {
  const size_t sz = argc;
  int test_arr[sz];
  fprintf(stdout, "size is %d\n", sizeof(test_arr));
  return 0;
}

If not, this change is necessary.
It probably won't, according to your previous comment about compile-time.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not compile: "error C2131: expression did not evaluate to a constant"
The compiler complains about [sz].

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @hliu18 for verifying this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see this PR was imported but not checked in due to conflicts. Can someone help move this PR forward? I was going to fix a couple of build break on MSVC and noticed this.

new uint64_t[reuse_vector_size * reuse_vector_size]);

for (uint64_t start_time = 0; start_time < reuse_vector_size; start_time++) {
// Initialize the reuse_table.
const auto offset = start_time * reuse_vector_size;
for (uint64_t i = 0; i < reuse_vector_size; i++) {
reuse_table[start_time][i] = 0;
reuse_table[offset + i] = 0;
}
// Examine all blocks.
for (auto const& block : block_accessed) {
Expand All @@ -1222,7 +1227,7 @@ void BlockCacheTraceAnalyzer::WriteBlockReuseTimeline(
// This block is accessed at start time and at the current time. We
// increment reuse_table[start_time][i] since it is reused at the ith
// window.
reuse_table[start_time][i]++;
reuse_table[offset + i]++;
}
}
}
Expand All @@ -1245,13 +1250,15 @@ void BlockCacheTraceAnalyzer::WriteBlockReuseTimeline(
out << header << std::endl;
for (uint64_t start_time = 0; start_time < reuse_vector_size; start_time++) {
std::string row(std::to_string(start_time * reuse_window));
const auto offset = start_time * reuse_vector_size;
for (uint64_t j = 0; j < reuse_vector_size; j++) {
row += ",";
if (j < start_time) {
row += "100.0";
} else {
row += std::to_string(percent(reuse_table[start_time][j],
reuse_table[start_time][start_time]));
row += std::to_string(percent(
reuse_table[offset + j],
reuse_table[offset + start_time]));
}
}
out << row << std::endl;
Expand Down Expand Up @@ -1680,7 +1687,8 @@ void BlockCacheTraceAnalyzer::PrintAccessCountStats(bool user_access_only,
for (auto const& caller_access : block->caller_num_access_map) {
if (!user_access_only ||
(user_access_only && is_user_access(caller_access.first))) {
caller_naccesses[caller_access.first] += caller_access.second;
caller_naccesses[caller_access.first] +=
static_cast<uint32_t>(caller_access.second);
naccesses += caller_access.second;
}
}
Expand Down