Bug 80358

Summary: [Chromium] IndexedDB: V8LocalContext creation in IDBKey extraction/injection is slow
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: New BugsAssignee: Joshua Bell <jsbell>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, adamk, arv, dglazkov, dgrogan, dslomov, japhet, levin+threading, michaeln, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Joshua Bell 2012-03-05 17:19:39 PST
[Chromium] IndexedDB: V8LocalContext creation in IDBKey extraction/injection is slow
Comment 1 Joshua Bell 2012-03-05 17:21:14 PST
Created attachment 130244 [details]
Patch
Comment 2 Joshua Bell 2012-03-05 17:24:15 PST
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.
Comment 3 Dmitry Lomov 2012-03-06 10:02:47 PST
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?
Comment 4 Joshua Bell 2012-03-06 11:55:39 PST
(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
Comment 5 Joshua Bell 2012-03-06 15:27:36 PST
Created attachment 130451 [details]
Patch
Comment 6 Joshua Bell 2012-03-06 15:35:30 PST
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.
Comment 7 Dmitry Lomov 2012-03-06 15:42:43 PST
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)
Comment 8 Joshua Bell 2012-03-06 16:12:57 PST
Created attachment 130461 [details]
Patch
Comment 9 Joshua Bell 2012-03-06 16:14:02 PST
(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.
Comment 10 Dmitry Lomov 2012-03-06 16:20:39 PST
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)
Comment 11 Joshua Bell 2012-03-06 16:57:44 PST
(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
Comment 12 Joshua Bell 2012-03-06 17:05:09 PST
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.
Comment 13 Joshua Bell 2012-03-06 17:13:07 PST
(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.
Comment 14 Dmitry Lomov 2012-03-06 17:23:04 PST
(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.
Comment 15 WebKit Review Bot 2012-03-07 03:14:51 PST
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
Comment 16 Joshua Bell 2012-03-07 11:35:54 PST
Created attachment 130658 [details]
Patch
Comment 17 Joshua Bell 2012-03-07 11:37:32 PST
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?
Comment 18 Dmitry Lomov 2012-03-07 11:57:47 PST
Comment on attachment 130658 [details]
Patch

Still LGTM :)
Comment 19 WebKit Review Bot 2012-03-07 14:30:05 PST
Comment on attachment 130658 [details]
Patch

Clearing flags on attachment: 130658

Committed r110104: <http://trac.webkit.org/changeset/110104>
Comment 20 WebKit Review Bot 2012-03-07 14:30:11 PST
All reviewed patches have been landed.  Closing bug.