From 4805fa0eae758bf3b5e5623cd81b4bf36af16618 Mon Sep 17 00:00:00 2001 From: Assaf Sela Date: Wed, 23 Sep 2015 14:25:46 -0700 Subject: [PATCH] Remove ldb HexToString method's usage of sscanf Summary: Fix hex2String performance issues by removing sscanf dependency. Also fixed some edge case handling (odd length, bad input). Test Plan: Created a test file which called old and new implementation, and validated results are the same. I'll paste results in the phabricator diff. Reviewers: igor, rven, anthony, IslamAbdelRahman, kradhakrishnan, yhchiang, sdong Reviewed By: sdong Subscribers: thatsafunnyname, leveldb, dhruba Differential Revision: https://reviews.facebook.net/D46785 --- CMakeLists.txt | 1 + Makefile | 6 +++++- src.mk | 3 ++- util/ldb_cmd.h | 32 +++++++++++++++++++++++++------ util/ldb_cmd_test.cc | 44 +++++++++++++++++++++++++++++++++++++++++++ util/sst_dump_tool.cc | 21 ++------------------- 6 files changed, 80 insertions(+), 27 deletions(-) create mode 100644 util/ldb_cmd_test.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 68624d274..e77db149c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -322,6 +322,7 @@ set(TESTS util/file_reader_writer_test.cc util/heap_test.cc util/histogram_test.cc + util/ldb_cmd_test.cc util/manual_compaction_test.cc util/memenv_test.cc util/mock_env_test.cc diff --git a/Makefile b/Makefile index 68e5ac59e..08f3be950 100644 --- a/Makefile +++ b/Makefile @@ -306,7 +306,8 @@ TESTS = \ heap_test \ compact_on_deletion_collector_test \ compaction_job_stats_test \ - transaction_test + transaction_test \ + ldb_cmd_test SUBSET := $(shell echo $(TESTS) |sed s/^.*$(ROCKSDBTESTS_START)/$(ROCKSDBTESTS_START)/) @@ -933,6 +934,9 @@ transaction_test: utilities/transactions/transaction_test.o $(LIBOBJECTS) $(TEST sst_dump: tools/sst_dump.o $(LIBOBJECTS) $(AM_LINK) +ldb_cmd_test: util/ldb_cmd_test.o $(LIBOBJECTS) $(TESTHARNESS) + $(AM_LINK) + ldb: tools/ldb.o $(LIBOBJECTS) $(AM_LINK) diff --git a/src.mk b/src.mk index 1fc4c139d..f1d9154f7 100644 --- a/src.mk +++ b/src.mk @@ -259,7 +259,8 @@ TEST_BENCH_SOURCES = \ util/testharness.cc \ util/testutil.cc \ util/thread_list_test.cc \ - util/thread_local_test.cc + util/thread_local_test.cc \ + util/ldb_cmd_test.cc JNI_NATIVE_SOURCES = \ java/rocksjni/backupenginejni.cc \ diff --git a/util/ldb_cmd.h b/util/ldb_cmd.h index 456751ec9..d48fcf667 100644 --- a/util/ldb_cmd.h +++ b/util/ldb_cmd.h @@ -130,18 +130,38 @@ public: } static string HexToString(const string& str) { + std::string::size_type len = str.length(); string parsed; - if (str[0] != '0' || str[1] != 'x') { + static const char* const hexas = "0123456789ABCDEF"; + parsed.reserve(len / 2); + + if (len < 2 || str[0] != '0' || str[1] != 'x') { fprintf(stderr, "Invalid hex input %s. Must start with 0x\n", str.c_str()); throw "Invalid hex input"; } - for (unsigned int i = 2; i < str.length();) { - int c; - sscanf(str.c_str() + i, "%2X", &c); - parsed.push_back(c); - i += 2; + for (unsigned int i = 2; i < len; i += 2) { + char a = static_cast(toupper(str[i])); + const char* p = std::lower_bound(hexas, hexas + 16, a); + if (*p != a) { + throw "Invalid hex value"; + } + + if (i + 1 >= len) { + // if odd number of chars than we just hit end of string + parsed.push_back(p - hexas); + break; + } + + char b = static_cast(toupper(str[i + 1])); + const char* q = std::lower_bound(hexas, hexas + 16, b); + if (*q == b) { + // pairwise compute decimal value from hex + parsed.push_back(((p - hexas) << 4) | (q - hexas)); + } else { + throw "Invalid hex value"; + } } return parsed; } diff --git a/util/ldb_cmd_test.cc b/util/ldb_cmd_test.cc new file mode 100644 index 000000000..c918cf565 --- /dev/null +++ b/util/ldb_cmd_test.cc @@ -0,0 +1,44 @@ +// Copyright (c) 2013, Facebook, Inc. All rights reserved. +// This source code is licensed under the BSD-style license found in the +// LICENSE file in the root directory of this source tree. An additional grant +// of patent rights can be found in the PATENTS file in the same directory. +// +#include "util/ldb_cmd.h" +#include "util/testharness.h" + +class LdbCmdTest : public testing::Test {}; + +TEST_F(LdbCmdTest, HexToString) { + // map input to expected outputs. + map> inputMap = { + {"0x7", {7}}, {"0x5050", {80, 80}}, {"0xFF", {-1}}, + {"0x1234", {18, 52}}, {"0xaa", {-86}}, {"0x123", {18, 3}}, + }; + + for (const auto& inPair : inputMap) { + auto actual = rocksdb::LDBCommand::HexToString(inPair.first); + auto expected = inPair.second; + for (unsigned int i = 0; i < actual.length(); i++) { + ASSERT_EQ(expected[i], static_cast(actual[i])); + } + } +} + +TEST_F(LdbCmdTest, HexToStringBadInputs) { + const vector badInputs = { + "0xZZ", "123", "0xx5", "0x11G", "Ox12", "0xT", "0x1Q1", + }; + for (const auto badInput : badInputs) { + try { + rocksdb::LDBCommand::HexToString(badInput); + std::cerr << "Should fail on bad hex value: " << badInput << "\n"; + FAIL(); + } catch (...) { + } + } +} + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/util/sst_dump_tool.cc b/util/sst_dump_tool.cc index 540ca6f40..2e319018a 100644 --- a/util/sst_dump_tool.cc +++ b/util/sst_dump_tool.cc @@ -329,23 +329,6 @@ void print_help() { " [--show_compression_sizes [--set_block_size=]]\n"); } -string HexToString(const string& str) { - string parsed; - if (str[0] != '0' || str[1] != 'x') { - fprintf(stderr, "Invalid hex input %s. Must start with 0x\n", - str.c_str()); - throw "Invalid hex input"; - } - - for (unsigned int i = 2; i < str.length();) { - int c; - sscanf(str.c_str() + i, "%2X", &c); - parsed.push_back(c); - i += 2; - } - return parsed; -} - } // namespace int SSTDumpTool::Run(int argc, char** argv) { @@ -409,10 +392,10 @@ int SSTDumpTool::Run(int argc, char** argv) { if (input_key_hex) { if (has_from) { - from_key = HexToString(from_key); + from_key = rocksdb::LDBCommand::HexToString(from_key); } if (has_to) { - to_key = HexToString(to_key); + to_key = rocksdb::LDBCommand::HexToString(to_key); } }