(No longer employed by Mozilla) Aaron Klotz Assignee
	Description • 12 years ago

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.

	(No longer employed by Mozilla) Aaron Klotz Assignee
	Comment 1 • 12 years ago

This version sets a marker whenever NSPR or SQLite I/O occurs on the main thread.

	(No longer employed by Mozilla) Aaron Klotz 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.

	Benoit Girard (:BenWa)
	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.

	(No longer employed by Mozilla) Aaron Klotz 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.

	Benoit Girard (:BenWa)
	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.

	(No longer employed by Mozilla) Aaron Klotz Assignee
	Comment 6 • 11 years ago

Updated patch with changes, carrying forward r+

	(No longer employed by Mozilla) Aaron Klotz 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/1a0ff6b97e03