Bug 80358 – [Chromium] IndexedDB: V8LocalContext creation in IDBKey extraction/injection is slow

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Joshua Bell]

[Chromium] IndexedDB: V8LocalContext creation in IDBKey extraction/injection is slow

[Joshua Bell]

Created attachment 130244 [details]
Patch

[Joshua Bell]

Dmitry, can you take a pre-review look at this CL and see if seems at all reasonable?

When profiling with chrome://tracing the V8LocalContext creation was taking more than 90% of the time in these functions, and these functions represent the bulk of the time spent in the IndexedDB calls. With this change, time spent in IPC (renderer->browser, browser->utility) becomes the dominant factor. I don't have benchmarks yet, however.

[Dmitry Lomov]

Comment on attachment 130244 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130244&action=review

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:173
> +        AtomicallyInitializedStatic(ThreadSpecific<PersistentContext>*, instance = new ThreadSpecific<PersistentContext>);

I am not too happy about this. This will break if we will ever execute under 2 different v8 isolates on the same thread (you cannot share contexts between isolates because they have different heaps). Admittedly  we do not do this today :)
One solution to that would be to cache the context in V8BindingPerIsolateData instead. What do you think?

[Joshua Bell]

(In reply to comment #3)
> 
> I am not too happy about this. This will break if we will ever execute under 2 different v8 isolates on the same thread (you cannot share contexts between isolates because they have different heaps). Admittedly  we do not do this today :)
> One solution to that would be to cache the context in V8BindingPerIsolateData instead. What do you think?

I have no preference as to where we should cache the context; I'll look into V8BindingPerIsolateData

[Joshua Bell]

Created attachment 130451 [details]
Patch

[Joshua Bell]

With a simple benchmark (1000 puts of an object into a store using a keyPath), the both the put() operations and the transaction tasks saw a 3-4x speedup.

[Dmitry Lomov]

Comment on attachment 130451 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130451&action=review

LGTM

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:151
> +class V8AuxiliaryContext {

Remove V8LocalContext and replace all usages with V8AuxiliaryContext?
(I see that the only other usages are in IDB tests)

[Joshua Bell]

Created attachment 130461 [details]
Patch

[Joshua Bell]

(In reply to comment #7)
> > Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:151
> > +class V8AuxiliaryContext {
> 
> Remove V8LocalContext and replace all usages with V8AuxiliaryContext?
> (I see that the only other usages are in IDB tests)

Ah, nice. *delete* *delete*

Also clear out the context explicitly in the V8BindingPerIsolateData destructor.

[Dmitry Lomov]

Comment on attachment 130461 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130461&action=review

Still LGTM :)

> Source/WebCore/bindings/v8/V8Binding.cpp:64
> +        m_auxiliaryContext.Dispose();

Not strictly necessary since isolate is going down anyway but nice to have

> Source/WebCore/bindings/v8/V8Utilities.cpp:68
> +    V8BindingPerIsolateData::ensureInitialized(v8::Isolate::GetCurrent());

Tests won't pass without this?
(if they won't let's have it)

[Joshua Bell]

(In reply to comment #10)
> (From update of attachment 130461 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130461&action=review
> 
> Still LGTM :)
> 
> > Source/WebCore/bindings/v8/V8Binding.cpp:64
> > +        m_auxiliaryContext.Dispose();
> 
> Not strictly necessary since isolate is going down anyway but nice to have

Ah, right - even Persistent<> handles are necessarily scoped to an isolate. (Learning as I go.)

> > Source/WebCore/bindings/v8/V8Utilities.cpp:68
> > +    V8BindingPerIsolateData::ensureInitialized(v8::Isolate::GetCurrent());
> 
> Tests won't pass without this?
> (if they won't let's have it)

Layout tests (under DRT) were hitting this w/o it:

# Fatal error in ../../v8/src/isolate.h, line 439
# CHECK(isolate != __null) failed

[Joshua Bell]

Comment on attachment 130461 [details]
Patch

Hrm, now I'm seeing DRT crash on that check *with* that line. I must have mucked something up, so removing the r? for now.

[Joshua Bell]

(In reply to comment #12)
> (From update of attachment 130461 [details])
> Hrm, now I'm seeing DRT crash on that check *with* that line. I must have mucked something up, so removing the r? for now.

Huh, it was actually the destructor addition intermittently hitting that CHECK during the layout tests. Removed both, but need to run the full suite of tests before re-upping.

[Dmitry Lomov]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 130461 [details] [details])
> > Hrm, now I'm seeing DRT crash on that check *with* that line. I must have mucked something up, so removing the r? for now.
> 
> Huh, it was actually the destructor addition intermittently hitting that CHECK during the layout tests. Removed both, but need to run the full suite of tests before re-upping.

Oh right the destructor is called *after* isolate exited.

Make sure that IDB unit test runs without ensureInitialized as well.

[WebKit Review Bot]

Comment on attachment 130461 [details]
Patch

Attachment 130461 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11837943

New failing tests:
storage/indexeddb/objectstore-count.html
storage/indexeddb/index-count.html

[Joshua Bell]

Created attachment 130658 [details]
Patch

[Joshua Bell]

Okay, this time for sure. Removed the cargo-cultisms. Specifically layout tests and webkit_unit_tests (IDBKeyFromValueAndKeyPathTest.*) that were concerns before, as well as the usual suite.

abarth@ or tony@, r?

[Dmitry Lomov]

Comment on attachment 130658 [details]
Patch

Still LGTM :)

[WebKit Review Bot]

Comment on attachment 130658 [details]
Patch

Clearing flags on attachment: 130658

Committed r110104: <http://trac.webkit.org/changeset/110104>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.