861287 - Integrate IndexedDB into the gecko profiler

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Primary patch, v1

We've had lots of reports about slow database performance that turned out to be inaccurate. Oftentimes these assumptions have prevented us from hunting for and seeing less obvious bottlenecks. I'd like to make it very easy for developers to see where indexedDB is taking time so that we can focus our performance efforts.

The first patch here is the basic helper stuff to get these annotations into the tree.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Full patch

This is the full version that I'll check in, with all the annotations.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 736888 [details] [diff] [review]
Full patch

Review of attachment 736888 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the changes we discussed on IRC

::: dom/indexedDB/IDBCursor.cpp
@@ +1026,5 @@
>  
>  nsresult
>  ContinueHelper::PackArgumentsForParentProcess(CursorRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

Thread-safety assertion.

@@ +1050,1 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Move this above.

::: dom/indexedDB/IDBIndex.cpp
@@ +1191,5 @@
>  
>  nsresult
>  GetKeyHelper::PackArgumentsForParentProcess(IndexRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

Thread-safety assertion.

@@ +1216,1 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Move this above.

@@ +1321,5 @@
>  
>  nsresult
>  GetHelper::PackArgumentsForParentProcess(IndexRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

Thread-safety assertion.

@@ +1345,1 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Move this above.

@@ +1520,5 @@
>  
>  nsresult
>  GetAllKeysHelper::PackArgumentsForParentProcess(IndexRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

Thread-safety assertion.

@@ +1552,1 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Move this above.

@@ +1674,5 @@
>  
>  nsresult
>  GetAllHelper::PackArgumentsForParentProcess(IndexRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

Thread-safety assertion.

@@ +1706,1 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Move this above.

@@ +2011,5 @@
>  
>  nsresult
>  OpenKeyCursorHelper::PackArgumentsForParentProcess(IndexRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

...

@@ +2043,1 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

...

@@ +2350,5 @@
>  
>  nsresult
>  OpenCursorHelper::PackArgumentsForParentProcess(IndexRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

...

@@ +2382,1 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

...

@@ +2538,5 @@
>  
>  nsresult
>  CountHelper::PackArgumentsForParentProcess(IndexRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

...

@@ +2568,1 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

...

::: dom/indexedDB/IDBObjectStore.cpp
@@ +3121,5 @@
>  
>  nsresult
>  AddHelper::PackArgumentsForParentProcess(ObjectStoreRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

Add a thread-safety assertion?

@@ +3181,1 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Move this above the profiler label?

@@ +3283,5 @@
>  
>  nsresult
>  GetHelper::PackArgumentsForParentProcess(ObjectStoreRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

Thread-safety assertion?

@@ +3300,5 @@
>  
>  AsyncConnectionHelper::ChildProcessSendResult
>  GetHelper::SendResponseToChildProcess(nsresult aResultCode)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

And here.

@@ +3418,5 @@
>  
>  nsresult
>  DeleteHelper::PackArgumentsForParentProcess(ObjectStoreRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

And here.

@@ +3435,5 @@
>  
>  AsyncConnectionHelper::ChildProcessSendResult
>  DeleteHelper::SendResponseToChildProcess(nsresult aResultCode)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

And here.

@@ +3498,5 @@
>  
>  nsresult
>  ClearHelper::PackArgumentsForParentProcess(ObjectStoreRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

And here.

@@ +3517,1 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Move this above the label?

@@ +3729,5 @@
>  nsresult
>  OpenCursorHelper::PackArgumentsForParentProcess(
>                                                ObjectStoreRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

Thread-safety assertion.

@@ +3761,1 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Move this above.

@@ +4176,5 @@
>  
>  nsresult
>  GetAllHelper::PackArgumentsForParentProcess(ObjectStoreRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

Thread-safety assertion.

@@ +4208,1 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Move this above.

@@ +4382,5 @@
>  
>  nsresult
>  CountHelper::PackArgumentsForParentProcess(ObjectStoreRequestParams& aParams)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

Thread-safety assertion.

@@ +4412,1 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Move this above.

::: dom/indexedDB/IDBTransaction.cpp
@@ +827,5 @@
>  CommitHelper::Run()
>  {
>    if (NS_IsMainThread()) {
> +    PROFILER_MAIN_THREAD_LABEL("IndexedDB",
> +                               "IDBTransaction::CommitHelper::Run");

Missing filename.

@@ +895,5 @@
>  
>      return NS_OK;
>    }
>  
> +  PROFILER_LABEL("IndexedDB", "IDBTransaction::CommitHelper::Run");

Missing filename.

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +1569,1 @@
>      NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Move this above.

@@ +2199,5 @@
>  
>  nsresult
>  OpenDatabaseHelper::EnsureSuccessResult()
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

Thread-safety assertion

@@ +2340,5 @@
>  
>  void
>  OpenDatabaseHelper::DispatchSuccessEvent()
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

...

@@ +2359,5 @@
>  
>  void
>  OpenDatabaseHelper::DispatchErrorEvent()
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",

...

::: dom/indexedDB/TransactionThreadPool.cpp
@@ +156,5 @@
>  
>  nsresult
>  TransactionThreadPool::Cleanup()
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB", "TransactionThreadPool::Cleanup");

Missing filename.

@@ +420,5 @@
>  void
>  TransactionThreadPool::AbortTransactionsForDatabase(IDBDatabase* aDatabase)
>  {
> +  PROFILER_MAIN_THREAD_LABEL("IndexedDB",
> +                             "TransactionThreadPool::"

Missing filename.

::: dom/quota/QuotaManager.cpp
@@ +1885,5 @@
>  
>  NS_IMETHODIMP
>  OriginClearRunnable::Run()
>  {
> +  PROFILER_LABEL("Quota", "OriginClearRunnable::Run");

Why are the various parts of this runnable not separated?

@@ +2129,5 @@
>  
>  NS_IMETHODIMP
>  AsyncUsageRunnable::Run()
>  {
> +  PROFILER_LABEL("Quota", "AsyncUsageRunnable::Run");

Ditto.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Final patch

Initially I wasn't going to do the thread assertion stuff since it's already included in the macros, but after looking a little more I realized that our thread/process assertions have gotten a little inconsistent so I went ahead and made them all uniform.

I also removed the filenames like we discussed, and we talked about splitting the rare-ish runnables into MAIN_THREAD and non.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This can't land until bug 734691.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b13433d8d0c

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Backed out, gcc didn't like it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b13433d8d0c

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dbc29b317ec

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f0cf98b3cd for bustage

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8dbc29b317ec
https://hg.mozilla.org/mozilla-central/rev/f0f0cf98b3cd