Closed
Bug 867762
Opened 12 years ago
Closed 11 years ago
Interpose NSPR and SQLLite mainthread I/O
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: bugzilla, Assigned: bugzilla)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
This is an interim solution to allow us to expose mainthread I/O to the profiler. The plan is to do this in the short term, and then leverage the code used for write poisoning (bug 832076) to do this in the longer term. The data generated here will need to be inserted into a profile via the mechanism developed in bug 867757.
Assignee | ||
Comment 1•12 years ago
|
||
This version sets a marker whenever NSPR or SQLite I/O occurs on the main thread.
Assignee | ||
Comment 2•11 years ago
|
||
Corrected a minor bug in the previous version. This one still places markers whenever main thread I/O is encountered.
Attachment #753121 -
Attachment is obsolete: true
Attachment #755403 -
Flags: feedback?(bgirard)
Comment 3•11 years ago
|
||
Comment on attachment 755403 [details] [diff] [review] NSPR and SQLite interposing, marker edition Review of attachment 755403 [details] [diff] [review]: ----------------------------------------------------------------- Nice patch. It's in the right direction. My only major critic is making sure we're thread safe. This patch introduces nsresult for a new API that doesn't require it for legacy reason. It should just use bool. The code sets the error code but in reality they wont be handled differently so a bool return value is all we need. The profiler module needs to support being disabled using MOZ_ENABLE_PROFILER_SPS. I think this patch may break that. This is a good way to turn off profiling if it causes a serious problem or we're not happy with the performance hit. ::: tools/profiler/IOInterposer.cpp @@ +14,5 @@ > +/* static */ IOInterposer* > +IOInterposer::GetInstance() > +{ > + if (!sSingleton) { > + sSingleton = new IOInterposer(); Either add an assert to make sure this only happens on the main thread or an assert that the right lock is held. If it doesn't then this code isn't thread safe. The thread safety of the remaining code is worrying. @@ +41,5 @@ > +IOInterposer::Init() > +{ > + nsRefPtr<IOInterposerModule> nsprModule = new NSPRInterposer(); > + NS_ENSURE_TRUE(nsprModule, NS_ERROR_UNEXPECTED); > + nsresult rv = nsprModule->Init(this, IOInterposerSink::OpAll); Is there really a use case for interposing only certain IO calls? @@ +69,5 @@ > +void > +IOInterposer::Observe(IOInterposerSink::Operation aOp) > +{ > + const char* ops[] = {"none", "read", "write", "invalid", "fsync"}; > + PROFILER_MARKER(ops[aOp]); Later we will need to improve how we deal with these. But that's fine for now. ::: tools/profiler/IOInterposer.h @@ +10,5 @@ > +#include "mozilla/XPCOM.h" > + > +namespace mozilla { > + > +class IOInterposerSink Needs class comment. I'm not familiar with the term 'Sink', is it a synonym for Observer? What's the reason for separating IOInterposerSink and IOInterposer? @@ +19,5 @@ > + OpNone = 0, > + OpRead = 1, > + OpWrite = 2, > + OpFSync = 4, > + OpAll = 7 I think it's cleaner to use '1 << X'. @@ +24,5 @@ > + }; > + virtual void Observe(Operation aOp) = 0; > +}; > + > +class IOInterposerModule class comment @@ +29,5 @@ > +{ > +public: > + IOInterposerModule() {} > + virtual ~IOInterposerModule() {} > + virtual nsresult Init(IOInterposerSink* aSink, const uint32_t aOpsToInterpose) = 0; uint32_t/Operation @@ +30,5 @@ > +public: > + IOInterposerModule() {} > + virtual ~IOInterposerModule() {} > + virtual nsresult Init(IOInterposerSink* aSink, const uint32_t aOpsToInterpose) = 0; > + virtual nsresult Enable(bool aEnable) = 0; Either enable should be infallible or enable could use a bit of documentation. @@ +39,5 @@ > + IOInterposerModule(const IOInterposerModule&); > + IOInterposerModule& operator=(const IOInterposerModule&); > +}; > + > +class IOInterposer MOZ_FINAL : public IOInterposerSink class comment ::: tools/profiler/Makefile.in @@ +50,5 @@ > JSObjectBuilder.cpp \ > JSCustomObjectBuilder.cpp \ > + IOInterposer.cpp \ > + NSPRInterposer.cpp \ > + SQLiteInterposer.cpp \ I think this is already rotted. Small price to pay for a better build system down the road :) ::: tools/profiler/SQLiteInterposer.cpp @@ +47,5 @@ > +{ > + if (!sObject) { > + return; > + } > + NS_ASSERTION(sObject->mSink, "SQLiteInterposer not initialized!"); Trivial: IMO this is redundant since line 53 will always crash. @@ +48,5 @@ > + if (!sObject) { > + return; > + } > + NS_ASSERTION(sObject->mSink, "SQLiteInterposer not initialized!"); > + if ((sObject->mOps & IOInterposerSink::OpRead) && NS_IsMainThread()) { If this isn't called on the main thread we will race on sObject but otherwise silently drop the call? I'd rather have the main thread check at the start of these as a debug time assert.
Attachment #755403 -
Flags: feedback?(bgirard) → feedback+
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Benoit Girard (:BenWa) from comment #3) > Comment on attachment 755403 [details] [diff] [review] > NSPR and SQLite interposing, marker edition > > Review of attachment 755403 [details] [diff] [review]: > ----------------------------------------------------------------- > > The profiler module needs to support being disabled using > MOZ_ENABLE_PROFILER_SPS. I think this patch may break that. This is a good > way to turn off profiling if it causes a serious problem or we're not happy > with the performance hit. > Either the code doesn't get built without MOZ_ENABLE_PROFILER_SPS, or I've added #ifdefs as necessary to ensure that calling into these functions is a no-op (particularly for the SQLiteInterposer case). > Either add an assert to make sure this only happens on the main thread or an > assert that the right lock is held. If it doesn't then this code isn't > thread safe. The thread safety of the remaining code is worrying. I've added asserts where required. I've also documented the specific functions that are permitted to be called from non-main threads. > Is there really a use case for interposing only certain IO calls? In the case of the profiler, no, however my intention is to leave options open whereby we could reuse these interfaces for other purposes (such as write poisoning). > > @@ +69,5 @@ > > +void > > +IOInterposer::Observe(IOInterposerSink::Operation aOp) > > +{ > > + const char* ops[] = {"none", "read", "write", "invalid", "fsync"}; > > + PROFILER_MARKER(ops[aOp]); > > Later we will need to improve how we deal with these. But that's fine for > now. This has been updated to use the PROFILER_SAMPLE_IMMEDIATELY macros as implemented in bug 867757. > Needs class comment. Added everywhere. > I'm not familiar with the term 'Sink', is it a synonym for Observer? 'Sink' as in 'event source / event sink'. I've renamed everything to use "Observer" for consistency. > What's the reason for separating IOInterposerSink and IOInterposer? Again, this is to allow us to use the interfaces for other purposes. > > @@ +19,5 @@ > > + OpNone = 0, > > + OpRead = 1, > > + OpWrite = 2, > > + OpFSync = 4, > > + OpAll = 7 > > I think it's cleaner to use '1 << X'. Done. > > @@ +24,5 @@ > > + }; > > + virtual void Observe(Operation aOp) = 0; > > +}; > > + > > +class IOInterposerModule > > class comment > > @@ +29,5 @@ > > +{ > > +public: > > + IOInterposerModule() {} > > + virtual ~IOInterposerModule() {} > > + virtual nsresult Init(IOInterposerSink* aSink, const uint32_t aOpsToInterpose) = 0; > > uint32_t/Operation Done. > > @@ +30,5 @@ > > +public: > > + IOInterposerModule() {} > > + virtual ~IOInterposerModule() {} > > + virtual nsresult Init(IOInterposerSink* aSink, const uint32_t aOpsToInterpose) = 0; > > + virtual nsresult Enable(bool aEnable) = 0; > > Either enable should be infallible or enable could use a bit of > documentation. Yeah, Enable is basically infallible. I've changed Enable to return void. > > @@ +39,5 @@ > > + IOInterposerModule(const IOInterposerModule&); > > + IOInterposerModule& operator=(const IOInterposerModule&); > > +}; > > + > > +class IOInterposer MOZ_FINAL : public IOInterposerSink > > class comment > > ::: tools/profiler/Makefile.in > @@ +50,5 @@ > > JSObjectBuilder.cpp \ > > JSCustomObjectBuilder.cpp \ > > + IOInterposer.cpp \ > > + NSPRInterposer.cpp \ > > + SQLiteInterposer.cpp \ > > I think this is already rotted. Small price to pay for a better build system > down the road :) > Fixed, of course :) > ::: tools/profiler/SQLiteInterposer.cpp > @@ +47,5 @@ > > +{ > > + if (!sObject) { > > + return; > > + } > > + NS_ASSERTION(sObject->mSink, "SQLiteInterposer not initialized!"); > > Trivial: IMO this is redundant since line 53 will always crash. Done. > > @@ +48,5 @@ > > + if (!sObject) { > > + return; > > + } > > + NS_ASSERTION(sObject->mSink, "SQLiteInterposer not initialized!"); > > + if ((sObject->mOps & IOInterposerSink::OpRead) && NS_IsMainThread()) { > > If this isn't called on the main thread we will race on sObject but > otherwise silently drop the call? I'd rather have the main thread check at > the start of these as a debug time assert. I've moved the main thread check up to the start of those functions, however it can't be an assertion because we expect this to be called from other threads; that check filters out the I/O that we actually want to observe. I've also added comments to point out that NSPRInterposer and SQLiteInterposer should only be cleaned up during shutdown after other threads have been stopped.
Attachment #755403 -
Attachment is obsolete: true
Attachment #761107 -
Flags: review?(bgirard)
Comment 5•11 years ago
|
||
Comment on attachment 761107 [details] [diff] [review] NSPR and SQLite Main Thread I/O Interposing Review of attachment 761107 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the following changes. ::: tools/profiler/ProfilerIOInterposeObserver.cpp @@ +29,5 @@ > + const char* aModuleInfo) > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + const char* ops[] = {"none", "read", "write", "invalid", "fsync"}; > + PROFILER_SAMPLE_IMMEDIATELY3(ops[aOp], aDuration, aModuleInfo); I'm not sure this is a good idea. For now lets just use a marker like you had before and let leave this discussion for bug 867757. ::: tools/profiler/platform.cpp @@ +464,5 @@ > mozilla::AndroidBridge::Bridge()->StartJavaProfiling(javaInterval, 1000); > } > #endif > > + mozilla::IOInterposer::GetInstance()->Enable(true); I think we want to add an IO feature. Once we start unwinding the stack for each IO call we will be introducing a significant overhead in both runtime and storage overhead for watching IO. It's best to let users opt out of it. Look for get_features, it's very straight forward.
Attachment #761107 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 6•11 years ago
|
||
Updated patch with changes, carrying forward r+
Attachment #761107 -
Attachment is obsolete: true
Attachment #762776 -
Flags: review+
Assignee | ||
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a0ff6b97e03 Try build: https://tbpl.mozilla.org/?tree=Try&rev=801edc571645 Pull request for Profiler Addon changes merged in yesterday.
Comment 8•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1a0ff6b97e03
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•