From 8bfb4fe9cb4755f71fd33d85633f19eef7c703f3 Mon Sep 17 00:00:00 2001 From: Matt Glazar Date: Sun, 14 Apr 2019 20:42:25 -0700 Subject: [PATCH] Rename EdenStats to EdenThreadStats Summary: I'm confused by the naming of EdenStats and ThreadLocalEdenStats. For example, when I see "ThreadLocalEdenStats", I think that such objects can only be accessed by one thread. That's definitely not what "ThreadLocalEdenStats" means! I think the following naming scheme makes more sense: * **EdenThreadStats**: Statistics which are updated by one thread. Currently called EdenStats. * **EdenStats**: Statistics for all threads. Provides access to EdenThreadStats. Currently called ThreadLocalEdenStats. Implement half of my preferred scheme: rename EdenStats to EdenThreadStats. (Make this diff easier to read by renaming ThreadLocalEdenStats to EdenStats in a separate diff.) In effect, implement the following naming scheme: * **EdenThreadStats**: Statistics which are updated by one thread. (was EdenStats) * **ThreadLocalEdenStats**: Statistics for all threads. Provides access to EdenThreadStats. (no change) This diff should not change behavior. Reviewed By: simpkins Differential Revision: D14822274 fbshipit-source-id: 236cd9878cd249a06d14e124050ee01329667a18 --- eden/fs/fuse/Dispatcher.h | 5 +-- eden/fs/fuse/FuseChannel.cpp | 66 ++++++++++++++++++----------------- eden/fs/fuse/RequestData.cpp | 2 +- eden/fs/fuse/RequestData.h | 4 +-- eden/fs/tracing/EdenStats.cpp | 10 +++--- eden/fs/tracing/EdenStats.h | 23 ++++++------ 6 files changed, 58 insertions(+), 52 deletions(-) diff --git a/eden/fs/fuse/Dispatcher.h b/eden/fs/fuse/Dispatcher.h index 9a99039f3c..4a158c1d15 100644 --- a/eden/fs/fuse/Dispatcher.h +++ b/eden/fs/fuse/Dispatcher.h @@ -35,12 +35,13 @@ namespace eden { class DirList; class Dispatcher; -class EdenStats; +class EdenThreadStats; class EdenStatsTag; class RequestData; class FileHandle; class MountPoint; -using ThreadLocalEdenStats = folly::ThreadLocal; +using ThreadLocalEdenStats = + folly::ThreadLocal; class Dispatcher { fuse_init_out connInfo_; diff --git a/eden/fs/fuse/FuseChannel.cpp b/eden/fs/fuse/FuseChannel.cpp index 2c69f98434..2fafbef93b 100644 --- a/eden/fs/fuse/FuseChannel.cpp +++ b/eden/fs/fuse/FuseChannel.cpp @@ -175,43 +175,45 @@ void installSignalHandler() { struct FuseChannel::HandlerEntry { Handler handler; - EdenStats::HistogramPtr histogram; + EdenThreadStats::HistogramPtr histogram; }; const FuseChannel::HandlerMap FuseChannel::handlerMap_ = { - {FUSE_READ, {&FuseChannel::fuseRead, &EdenStats::read}}, - {FUSE_WRITE, {&FuseChannel::fuseWrite, &EdenStats::write}}, - {FUSE_LOOKUP, {&FuseChannel::fuseLookup, &EdenStats::lookup}}, - {FUSE_FORGET, {&FuseChannel::fuseForget, &EdenStats::forget}}, - {FUSE_GETATTR, {&FuseChannel::fuseGetAttr, &EdenStats::getattr}}, - {FUSE_SETATTR, {&FuseChannel::fuseSetAttr, &EdenStats::setattr}}, - {FUSE_READLINK, {&FuseChannel::fuseReadLink, &EdenStats::readlink}}, - {FUSE_SYMLINK, {&FuseChannel::fuseSymlink, &EdenStats::symlink}}, - {FUSE_MKNOD, {&FuseChannel::fuseMknod, &EdenStats::mknod}}, - {FUSE_MKDIR, {&FuseChannel::fuseMkdir, &EdenStats::mkdir}}, - {FUSE_UNLINK, {&FuseChannel::fuseUnlink, &EdenStats::unlink}}, - {FUSE_RMDIR, {&FuseChannel::fuseRmdir, &EdenStats::rmdir}}, - {FUSE_RENAME, {&FuseChannel::fuseRename, &EdenStats::rename}}, - {FUSE_LINK, {&FuseChannel::fuseLink, &EdenStats::link}}, - {FUSE_OPEN, {&FuseChannel::fuseOpen, &EdenStats::open}}, - {FUSE_STATFS, {&FuseChannel::fuseStatFs, &EdenStats::statfs}}, - {FUSE_RELEASE, {&FuseChannel::fuseRelease, &EdenStats::release}}, - {FUSE_FSYNC, {&FuseChannel::fuseFsync, &EdenStats::fsync}}, - {FUSE_SETXATTR, {&FuseChannel::fuseSetXAttr, &EdenStats::setxattr}}, - {FUSE_GETXATTR, {&FuseChannel::fuseGetXAttr, &EdenStats::getxattr}}, - {FUSE_LISTXATTR, {&FuseChannel::fuseListXAttr, &EdenStats::listxattr}}, + {FUSE_READ, {&FuseChannel::fuseRead, &EdenThreadStats::read}}, + {FUSE_WRITE, {&FuseChannel::fuseWrite, &EdenThreadStats::write}}, + {FUSE_LOOKUP, {&FuseChannel::fuseLookup, &EdenThreadStats::lookup}}, + {FUSE_FORGET, {&FuseChannel::fuseForget, &EdenThreadStats::forget}}, + {FUSE_GETATTR, {&FuseChannel::fuseGetAttr, &EdenThreadStats::getattr}}, + {FUSE_SETATTR, {&FuseChannel::fuseSetAttr, &EdenThreadStats::setattr}}, + {FUSE_READLINK, {&FuseChannel::fuseReadLink, &EdenThreadStats::readlink}}, + {FUSE_SYMLINK, {&FuseChannel::fuseSymlink, &EdenThreadStats::symlink}}, + {FUSE_MKNOD, {&FuseChannel::fuseMknod, &EdenThreadStats::mknod}}, + {FUSE_MKDIR, {&FuseChannel::fuseMkdir, &EdenThreadStats::mkdir}}, + {FUSE_UNLINK, {&FuseChannel::fuseUnlink, &EdenThreadStats::unlink}}, + {FUSE_RMDIR, {&FuseChannel::fuseRmdir, &EdenThreadStats::rmdir}}, + {FUSE_RENAME, {&FuseChannel::fuseRename, &EdenThreadStats::rename}}, + {FUSE_LINK, {&FuseChannel::fuseLink, &EdenThreadStats::link}}, + {FUSE_OPEN, {&FuseChannel::fuseOpen, &EdenThreadStats::open}}, + {FUSE_STATFS, {&FuseChannel::fuseStatFs, &EdenThreadStats::statfs}}, + {FUSE_RELEASE, {&FuseChannel::fuseRelease, &EdenThreadStats::release}}, + {FUSE_FSYNC, {&FuseChannel::fuseFsync, &EdenThreadStats::fsync}}, + {FUSE_SETXATTR, {&FuseChannel::fuseSetXAttr, &EdenThreadStats::setxattr}}, + {FUSE_GETXATTR, {&FuseChannel::fuseGetXAttr, &EdenThreadStats::getxattr}}, + {FUSE_LISTXATTR, + {&FuseChannel::fuseListXAttr, &EdenThreadStats::listxattr}}, {FUSE_REMOVEXATTR, - {&FuseChannel::fuseRemoveXAttr, &EdenStats::removexattr}}, - {FUSE_FLUSH, {&FuseChannel::fuseFlush, &EdenStats::flush}}, - {FUSE_OPENDIR, {&FuseChannel::fuseOpenDir, &EdenStats::opendir}}, - {FUSE_READDIR, {&FuseChannel::fuseReadDir, &EdenStats::readdir}}, - {FUSE_RELEASEDIR, {&FuseChannel::fuseReleaseDir, &EdenStats::releasedir}}, - {FUSE_FSYNCDIR, {&FuseChannel::fuseFsyncDir, &EdenStats::fsyncdir}}, - {FUSE_ACCESS, {&FuseChannel::fuseAccess, &EdenStats::access}}, - {FUSE_CREATE, {&FuseChannel::fuseCreate, &EdenStats::create}}, - {FUSE_BMAP, {&FuseChannel::fuseBmap, &EdenStats::bmap}}, + {&FuseChannel::fuseRemoveXAttr, &EdenThreadStats::removexattr}}, + {FUSE_FLUSH, {&FuseChannel::fuseFlush, &EdenThreadStats::flush}}, + {FUSE_OPENDIR, {&FuseChannel::fuseOpenDir, &EdenThreadStats::opendir}}, + {FUSE_READDIR, {&FuseChannel::fuseReadDir, &EdenThreadStats::readdir}}, + {FUSE_RELEASEDIR, + {&FuseChannel::fuseReleaseDir, &EdenThreadStats::releasedir}}, + {FUSE_FSYNCDIR, {&FuseChannel::fuseFsyncDir, &EdenThreadStats::fsyncdir}}, + {FUSE_ACCESS, {&FuseChannel::fuseAccess, &EdenThreadStats::access}}, + {FUSE_CREATE, {&FuseChannel::fuseCreate, &EdenThreadStats::create}}, + {FUSE_BMAP, {&FuseChannel::fuseBmap, &EdenThreadStats::bmap}}, {FUSE_BATCH_FORGET, - {&FuseChannel::fuseBatchForget, &EdenStats::forgetmulti}}, + {&FuseChannel::fuseBatchForget, &EdenThreadStats::forgetmulti}}, }; static iovec inline make_iovec(const void* addr, size_t len) { diff --git a/eden/fs/fuse/RequestData.cpp b/eden/fs/fuse/RequestData.cpp index 48af85c9a3..f0fd7fc4bb 100644 --- a/eden/fs/fuse/RequestData.cpp +++ b/eden/fs/fuse/RequestData.cpp @@ -53,7 +53,7 @@ RequestData& RequestData::create( void RequestData::startRequest( ThreadLocalEdenStats* stats, - EdenStats::HistogramPtr histogram) { + EdenThreadStats::HistogramPtr histogram) { startTime_ = steady_clock::now(); DCHECK(latencyHistogram_ == nullptr); latencyHistogram_ = histogram; diff --git a/eden/fs/fuse/RequestData.h b/eden/fs/fuse/RequestData.h index 215f9384ae..dfd29c215c 100644 --- a/eden/fs/fuse/RequestData.h +++ b/eden/fs/fuse/RequestData.h @@ -26,7 +26,7 @@ class RequestData : public folly::RequestData { fuse_in_header fuseHeader_; // Needed to track stats std::chrono::time_point startTime_; - EdenStats::HistogramPtr latencyHistogram_{nullptr}; + EdenThreadStats::HistogramPtr latencyHistogram_{nullptr}; ThreadLocalEdenStats* stats_{nullptr}; Dispatcher* dispatcher_{nullptr}; @@ -58,7 +58,7 @@ class RequestData : public folly::RequestData { void startRequest( ThreadLocalEdenStats* stats, - EdenStats::HistogramPtr histogram); + EdenThreadStats::HistogramPtr histogram); void finishRequest(); // Returns the associated dispatcher instance diff --git a/eden/fs/tracing/EdenStats.cpp b/eden/fs/tracing/EdenStats.cpp index af092f9159..f39bf270e6 100644 --- a/eden/fs/tracing/EdenStats.cpp +++ b/eden/fs/tracing/EdenStats.cpp @@ -26,9 +26,10 @@ constexpr std::chrono::microseconds kBucketSize{1000}; namespace facebook { namespace eden { -EdenStats::EdenStats() {} +EdenThreadStats::EdenThreadStats() {} -EdenStats::Histogram EdenStats::createHistogram(const std::string& name) { +EdenThreadStats::Histogram EdenThreadStats::createHistogram( + const std::string& name) { return Histogram{this, name, static_cast(kBucketSize.count()), @@ -41,14 +42,15 @@ EdenStats::Histogram EdenStats::createHistogram(const std::string& name) { } #if defined(EDEN_HAVE_STATS) -EdenStats::Timeseries EdenStats::createTimeseries(const std::string& name) { +EdenThreadStats::Timeseries EdenThreadStats::createTimeseries( + const std::string& name) { auto timeseries = Timeseries{this, name}; timeseries.exportStat(facebook::stats::COUNT); return timeseries; } #endif -void EdenStats::recordLatency( +void EdenThreadStats::recordLatency( HistogramPtr item, std::chrono::microseconds elapsed, std::chrono::seconds now) { diff --git a/eden/fs/tracing/EdenStats.h b/eden/fs/tracing/EdenStats.h index 5b886fa0b8..dc5c3e537e 100644 --- a/eden/fs/tracing/EdenStats.h +++ b/eden/fs/tracing/EdenStats.h @@ -21,29 +21,30 @@ namespace facebook { namespace eden { /** - * A tag class for using with folly::ThreadLocal when storing EdenStats. + * A tag class for using with folly::ThreadLocal when storing EdenThreadStats. */ class EdenStatsTag {}; -class EdenStats; +class EdenThreadStats; -using ThreadLocalEdenStats = folly::ThreadLocal; +using ThreadLocalEdenStats = + folly::ThreadLocal; /** - * EdenStats contains various thread-local stats structures. + * EdenThreadStats contains various thread-local stats structures. * - * Each EdenStats object should only be used from a single thread. - * The ThreadLocalEdenStats object should be used to maintain one EdenStats - * object for each thread that needs to access/update the stats. + * Each EdenThreadStats object should only be used from a single thread. + * The ThreadLocalEdenStats object should be used to maintain one + * EdenThreadStats object for each thread that needs to access/update the stats. */ -class EdenStats : public facebook::stats::ThreadLocalStatsT< - facebook::stats::TLStatsThreadSafe> { +class EdenThreadStats : public facebook::stats::ThreadLocalStatsT< + facebook::stats::TLStatsThreadSafe> { public: using Histogram = TLHistogram; #if defined(EDEN_HAVE_STATS) using Timeseries = TLTimeseries; #endif - explicit EdenStats(); + explicit EdenThreadStats(); // We track latency in units of microseconds, hence the _us suffix // in the histogram names below. @@ -99,7 +100,7 @@ class EdenStats : public facebook::stats::ThreadLocalStatsT< // thread from the one used to initiate it, we use HistogramPtr // as a helper for referencing the pointer-to-member that we // want to update at the end of the request. - using HistogramPtr = Histogram EdenStats::*; + using HistogramPtr = Histogram EdenThreadStats::*; /** Record a the latency for an operation. * item is the pointer-to-member for one of the histograms defined