673473 - Remove off-main-thread JSRuntime use in GetKeyPathValueFromStructuredData

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Luke Wagner [:luke]]

Spun off from bug 668915.

Comment 1

[Luke Wagner [:luke]]

IIUC, the simplest immediate solution is to associate a new runtime with the IDB thread and use this in GetKeyPathValueFromStructuredData.  This code is, like, really pretty nice and readable (hg annotate says kudos to bent), so I'm going to take a crack at it unless someone else is itching to.

Comment 2

[Luke Wagner [:luke]]

Attachment: patch

I was hoping to associate the JSRuntime/JSContext with some long-lived-but-single-threaded IndexedDB object, but I couldn't find one.  Lacking that, the simplest thing seems to be to just to use TLS and rely on the destroy hook to clean up.  With this patch, none of my single-threaded-runtime asserts hit for tests/dom/indexedDB.

One note about the patch: I added ThreadLocalJSRuntime (instead of just storing cx as the TLS value) so that we'd have a place to hold the global object when we get around to bug 604813.

Comment 3

[Luke Wagner [:luke]]

Attachment: patch

On second thought, abort() may be a bit harsh.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 548916 [details] [diff] [review]
patch

Oops, wrong bug.

Comment 5

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

Comment on attachment 548316 [details] [diff] [review]
patch

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

Looks great, thanks for tackling this!

::: dom/indexedDB/IDBObjectStore.cpp
@@ +241,5 @@
>  
>  class CreateIndexHelper : public AsyncConnectionHelper
>  {
>  public:
> +  CreateIndexHelper(IDBTransaction* aTransaction, IDBIndex* aIndex);

I think you can still keep this inlined, right?

@@ +264,5 @@
>  
>  private:
>    nsresult InsertDataFromObjectStore(mozIStorageConnection* aConnection);
>  
> +  static void DestroyTLSEntry(void* ptr);

Nit: params get 'aPtr' style names

@@ +2197,5 @@
>    return WrapNative(aCx, cursor, aVal);
>  }
>  
> +static const PRUintn BAD_TLS_INDEX = (PRUint32)-1;
> +PRUintn CreateIndexHelper::sTLSIndex = BAD_TLS_INDEX;

Can you move these up right after the class definition?

@@ +2201,5 @@
> +PRUintn CreateIndexHelper::sTLSIndex = BAD_TLS_INDEX;
> +
> +class ThreadLocalJSRuntime
> +{
> +  JSRuntime *mRuntime;

Nit: * goes on the left here ;)

@@ +2210,5 @@
> +  static const unsigned sRuntimeHeapSize = 64 * 1024;  // should be enough for anyone
> +
> +  ThreadLocalJSRuntime() : mRuntime(NULL), mContext(NULL), mGlobal(NULL) {}
> +
> +  nsresult Init()

This can be private. Also, since the error code isn't really important you could just make this return boolean...

@@ +2259,5 @@
> +  }
> +};
> +
> +JSClass ThreadLocalJSRuntime::sGlobalClass = {
> +  "IndexedDBOffThreadGlobal",

Nit: s/Off/Transaction/

@@ +2269,5 @@
> +CreateIndexHelper::CreateIndexHelper(IDBTransaction* aTransaction,
> +                                     IDBIndex* aIndex)
> +  : AsyncConnectionHelper(aTransaction, nsnull), mIndex(aIndex)
> +{
> +  if (sTLSIndex == BAD_TLS_INDEX)

Nit: Please brace single line if statements.

@@ +2409,5 @@
>      PRUint32 dataLength;
>      rv = stmt->GetSharedBlob(1, &dataLength, &data);
>      NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
>  
> +    NS_ENSURE_TRUE(sTLSIndex != BAD_TLS_INDEX, NS_ERROR_UNEXPECTED);

Error codes returned from this function have to be NS_ERROR_DOM_INDEXEDDB_* errors. Probably want NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR here.

Nit: newline after this

@@ +2415,5 @@
> +      reinterpret_cast<ThreadLocalJSRuntime*>(PR_GetThreadPrivate(sTLSIndex));
> +
> +    if (!tlsEntry) {
> +      tlsEntry = ThreadLocalJSRuntime::Create();
> +      NS_ENSURE_TRUE(tlsEntry, NS_ERROR_OUT_OF_MEMORY);

Same here, and a newline too!

Comment 6

[Luke Wagner [:luke]]

(In reply to comment #5)
> > +  CreateIndexHelper(IDBTransaction* aTransaction, IDBIndex* aIndex);
> 
> I think you can still keep this inlined, right?

I could but I thought it would be more readable if the TLS logic in the body was next to all the other TLS logic.  Does this sway you?

> > +  nsresult Init()
> 
> This can be private.

It is :)

> Also, since the error code isn't really important you
> could just make this return boolean...

Sounds great.  I would've gone with bool but I found a few places using nsresult instead of bool (e.g. TransactionThreadPool::GetOrCreate) and I thought it might be a style thing.

Everything else fixed; thanks!

Comment 7

[Luke Wagner [:luke]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e931fb581aee

I just realized I pushed without waiting for the answer to my question in comment 6.  If the answer is not 'yes', I can change it.

Comment 8

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

It's fine :)

Comment 9

[Luke Wagner [:luke]]

\o/

Comment 10

[Marco Bonardo [:mak]]

backed out due to Trace Malloc Leaks increase 1.61% on MacOSX 10.5.2. Perf-o-matic seems to clearly point at this changeset.
If this is expected or we are fine ignoring it, please repush specifying that.

Comment 11

[Luke Wagner [:luke]]

Marco, could I ask you a few basic questions:

By "Trace Malloc Leaks", do you mean the "LK: " number that shows up on TBPL for the "leak test build"?  I see this number hopping between 1.86MB and 1.89MB before and after the backed-out cset.  Is this what is being measured?

As an experiment, I put an abort() in the code I changed (which requires special indexedDB use to exercise) and ran:
  python leaktest.py -- --trace-malloc malloc.log --shutdown-leaks=sdleak.log
and the abort() wasn't hit.  Is that the right test to look at?

Lastly, mochitest-2 actually runs the tests which exercise this code, but looking at the processLeakLog() line in the logs doesn't report any leak nor does it appear to push any results to any graph server, so I don't think that's what this is about.

Comment 12

[Luke Wagner [:luke]]

So I also looked at the perftastic graph in question:
 http://graphs.mozilla.org/graph.html#tests=[[28,63,1287]]&sel=1311813637,1312202719

I do see it going from bouncing up and down to staying at the high level exactly at this patch's landing.  However, I see it going back to bouncing around well before the backout cset (f4415470040a).

I'll wait a day and see if anyone rejects, but I'm inclined to go with "ignore and reland".

Comment 13

[Marco Bonardo [:mak]]

Unfortunately I don't know the internals of all of our Talos tests that well, you may try pinging anode to get those. I'm not sure about the level of trust we have in this measure, especially on OSX10.5, may be we may ignore this kind of small noise or even we did in the past, I'm not the right person to do that call.

I did the backout because while this measure IS noisy, before this patch it was most of the time on the lower bound, and sometimes on the upper bound. After this patch that was inverted and was mostly on the upper bound. See (I suggest you to use the new graph server, is so much faster) http://graphs-new.mozilla.org/graph.html#tests=[[28,63,7]]&sel=1311952647785.9873,1312228051837&displayrange=7&datatype=running

Since there was a lot of backlog on inbound, and you were not around to ask if some sort of regressions was expected at that time, I preferred going the safe way.  After the backout this is still noisy as before, but I see more runs at the lower bound.

Comment 14

[Luke Wagner [:luke]]

Marco, thanks for your help on this and with inbound in general.  Wow, graphs-new is faster.

On more recent csets (after the backout), I'm still seeing a tendency to the high bound.  Based on this, above evidence and talking to bent and khuey, I'm going to reland the patch as is but, as bent suggested, with MOZ_COUNT_CTOR/DTOR added to ThreadLocalJSRuntime for good measure.

Comment 15

[Luke Wagner [:luke]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2cda40256a54

Comment 16

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/2cda40256a54