From 555ca3e7b7f06bd01dfd5e04dbb2cef5360f7917 Mon Sep 17 00:00:00 2001 From: Dmitri Smirnov Date: Wed, 22 Jul 2015 14:36:43 -0700 Subject: [PATCH] Fix WinEnv::NowMicrosec * std::chrono does not provide enough granularity for microsecs and periodically emits duplicates * the bug is manifested in log rotation logic where we get duplicate log file names and loose previous log content * msvc does not imlement COW on std::strings adjusted the test to use refs in the loops as auto does not retain ref info * adjust auto_log rotation test with Windows specific command to remove a folder. The test previously worked because we have unix utils installed in house but this may not be the case for everyone. --- port/win/env_win.cc | 13 ++++++++++--- util/auto_roll_logger.cc | 2 +- util/auto_roll_logger_test.cc | 27 +++++++++++++++++++-------- 3 files changed, 30 insertions(+), 12 deletions(-) diff --git a/port/win/env_win.cc b/port/win/env_win.cc index 2890ecc49..de31b0b9f 100644 --- a/port/win/env_win.cc +++ b/port/win/env_win.cc @@ -1976,9 +1976,16 @@ class WinEnv : public Env { } virtual uint64_t NowMicros() override { - using namespace std::chrono; - return duration_cast(system_clock::now().time_since_epoch()) - .count(); + // all std::chrono clocks on windows have the same resolution that is only + // On Windows 8 and Windows 2012 Server + // GetSystemTimePreciseAsFileTime(¤t_time) can be used + LARGE_INTEGER li; + QueryPerformanceCounter(&li); + // Convert to nanoseconds first to avoid loss of precision + // and divide by frequency + li.QuadPart *= std::micro::den; + li.QuadPart /= perf_counter_frequency_; + return li.QuadPart; } virtual uint64_t NowNanos() override { diff --git a/util/auto_roll_logger.cc b/util/auto_roll_logger.cc index 684abfc30..4ea035679 100644 --- a/util/auto_roll_logger.cc +++ b/util/auto_roll_logger.cc @@ -91,7 +91,7 @@ void AutoRollLogger::Logv(const char* format, va_list ap) { void AutoRollLogger::WriteHeaderInfo() { mutex_.AssertHeld(); - for (auto header : headers_) { + for (auto& header : headers_) { LogInternal("%s", header.c_str()); } } diff --git a/util/auto_roll_logger_test.cc b/util/auto_roll_logger_test.cc index cbcc4cf28..ef340630a 100644 --- a/util/auto_roll_logger_test.cc +++ b/util/auto_roll_logger_test.cc @@ -23,7 +23,17 @@ namespace rocksdb { class AutoRollLoggerTest : public testing::Test { public: static void InitTestDb() { - string deleteCmd = "rm -rf " + kTestDir; +#ifdef OS_WIN + // Replace all slashes in the path so windows CompSpec does not + // become confused + std::string testDir(kTestDir); + std::replace_if(testDir.begin(), testDir.end(), + [](char ch) { return ch == '/'; }, + '\\'); + std::string deleteCmd = "if exist " + testDir + " rd /s /q " + testDir; +#else + std::string deleteCmd = "rm -rf " + kTestDir; +#endif ASSERT_TRUE(system(deleteCmd.c_str()) == 0); Env::Default()->CreateDir(kTestDir); } @@ -296,17 +306,18 @@ TEST_F(AutoRollLoggerTest, InfoLogLevel) { // Test the logger Header function for roll over logs // We expect the new logs creates as roll over to carry the headers specified -static list GetOldFileNames(const string& path) { +static std::vector GetOldFileNames(const string& path) { + std::vector ret; + const string dirname = path.substr(/*start=*/ 0, path.find_last_of("/")); const string fname = path.substr(path.find_last_of("/") + 1); - vector children; + std::vector children; Env::Default()->GetChildren(dirname, &children); // We know that the old log files are named [path] // Return all entities that match the pattern - list ret; - for (auto child : children) { + for (auto& child : children) { if (fname != child && child.find(fname) == 0) { ret.push_back(dirname + "/" + child); } @@ -360,7 +371,7 @@ TEST_F(AutoRollLoggerTest, LogHeaderTest) { } } - const string& newfname = logger.TEST_log_fname().c_str(); + const string newfname = logger.TEST_log_fname(); // Log enough data to cause a roll over int i = 0; @@ -376,11 +387,11 @@ TEST_F(AutoRollLoggerTest, LogHeaderTest) { // Flush the log for the latest file LogFlush(&logger); - const list oldfiles = GetOldFileNames(newfname); + const auto oldfiles = GetOldFileNames(newfname); ASSERT_EQ(oldfiles.size(), (size_t) 2); - for (auto oldfname : oldfiles) { + for (auto& oldfname : oldfiles) { // verify that the files rolled over ASSERT_NE(oldfname, newfname); // verify that the old log contains all the header logs