Bug 200526 – Use one VM per thread for IDB serialization work in network process

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

[Sihui Liu]

Probably a memory improvement.

[Sihui Liu]

Created attachment 375774 [details]
Patch

[Sihui Liu]

*** Bug 192270 has been marked as a duplicate of this bug. ***

[Sihui Liu]

Created attachment 375775 [details]
Patch

[EWS Watchlist]

Comment on attachment 375775 [details]
Patch

Attachment 375775 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12876871

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/idbcursor_iterating.htm

[EWS Watchlist]

Created attachment 375783 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Sam Weinig]

Comment on attachment 375775 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        use lock, and the VM is created and destroyed on the same thread. This could be a memory improvement.

Is it a memory improvement?

[Sam Weinig]

Comment on attachment 375775 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        use lock, and the VM is created and destroyed on the same thread. This could be a memory improvement.

Is it a memory improvement?

[Geoffrey Garen]

These seem real:

Regressions: Unexpected crashes (3)
  imported/w3c/web-platform-tests/IndexedDB/idbcursor_iterating.htm [ Crash ]
  imported/w3c/web-platform-tests/IndexedDB/idbindex_getAll.html [ Crash ]
  storage/indexeddb/cursor-advance-workers.html [ Crash ]

[Sihui Liu]

Created attachment 376201 [details]
Patch

[Sihui Liu]

Created attachment 376211 [details]
Patch

[Sihui Liu]

(In reply to Sam Weinig from comment #6)
> Comment on attachment 375775 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375775&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        use lock, and the VM is created and destroyed on the same thread. This could be a memory improvement.
> 
> Is it a memory improvement?

This should be a memory improvement. If a domain opens multiple IDB databases, multiple SQLiteIDBBackingStores are created on the same thread, and each of them has a VM. 

What I learned is, each VM has its garbage collector and create extra threads for that. Cutting down the number of VMs should help reduce memory and the cost of initializing VMs.

[EWS Watchlist]

Comment on attachment 376211 [details]
Patch

Attachment 376211 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12906925

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-no-ftl

[Sam Weinig]

(In reply to Sihui Liu from comment #11)
> (In reply to Sam Weinig from comment #6)
> > Comment on attachment 375775 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=375775&action=review
> > 
> > > Source/WebCore/ChangeLog:10
> > > +        use lock, and the VM is created and destroyed on the same thread. This could be a memory improvement.
> > 
> > Is it a memory improvement?
> 
> This should be a memory improvement. If a domain opens multiple IDB
> databases, multiple SQLiteIDBBackingStores are created on the same thread,
> and each of them has a VM. 
> 
> What I learned is, each VM has its garbage collector and create extra
> threads for that. Cutting down the number of VMs should help reduce memory
> and the cost of initializing VMs.

Does it show up as an improvement on any memory benchmarks (even synthetic ones)?

[Sihui Liu]

Created attachment 376386 [details]
Test file

[Sihui Liu]

(In reply to Sam Weinig from comment #13)
> (In reply to Sihui Liu from comment #11)
> > (In reply to Sam Weinig from comment #6)
> > > Comment on attachment 375775 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=375775&action=review
> > > 
> > > > Source/WebCore/ChangeLog:10
> > > > +        use lock, and the VM is created and destroyed on the same thread. This could be a memory improvement.
> > > 
> > > Is it a memory improvement?
> > 
> > This should be a memory improvement. If a domain opens multiple IDB
> > databases, multiple SQLiteIDBBackingStores are created on the same thread,
> > and each of them has a VM. 
> > 
> > What I learned is, each VM has its garbage collector and create extra
> > threads for that. Cutting down the number of VMs should help reduce memory
> > and the cost of initializing VMs.
> 
> Does it show up as an improvement on any memory benchmarks (even synthetic
> ones)?

Attached a test file. It opens 100 databases and puts one item in each database one by one. It keeps the database objects in an array to avoid garbage collection.

I applied the patch on trunk r248706 and tested on release build. 

Using activity monitor to check memory usage of Safari Networking, without the change(one VM per thread) Safari Networking process used about 80MB memory; with the change it's about 34MB.

I didn't see difference in number of threads though.

[Sihui Liu]

Created attachment 376394 [details]
Patch

[Geoffrey Garen]

(In reply to Build Bot from comment #12)
> Comment on attachment 376211 [details]
> Patch
> 
> Attachment 376211 [details] did not pass jsc-ews (mac):
> Output: https://webkit-queues.webkit.org/results/12906925
> 
> New failing tests:
> mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-no-ftl

This was a spurious failure -- so you can use createContextGroup() after all.

[Geoffrey Garen]

> > Does it show up as an improvement on any memory benchmarks (even synthetic
> > ones)?
> 
> Attached a test file. It opens 100 databases and puts one item in each
> database one by one. It keeps the database objects in an array to avoid
> garbage collection.
> 
> I applied the patch on trunk r248706 and tested on release build. 
> 
> Using activity monitor to check memory usage of Safari Networking, without
> the change(one VM per thread) Safari Networking process used about 80MB
> memory; with the change it's about 34MB.
> 
> I didn't see difference in number of threads though.

Thanks for writing this test. I think it confirms a nice improvement.

Looks like our guess about thread count was wrong. That's OK, since our guess about memory use was right. Also a good reminder of the value of a test.

[Geoffrey Garen]

Comment on attachment 376394 [details]
Patch

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

r=me -- but please fix the bug I noticed before landing

> Source/WebCore/Modules/indexeddb/server/IDBSerializationContext.cpp:37
> +static HashMap<PAL::SessionID, IDBSerializationContext*>& serializationContextMap()

This map can be accessed by multiple threads (one SessionID per thread), so we need to guard it with a lock.

I'd suggest adding a global WTF::Lock, and using WTF::Locker for the full scope of the two functions below.

[Sihui Liu]

Created attachment 376441 [details]
Patch for landing

[Sihui Liu]

Created attachment 376443 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 376443 [details]
Patch for landing

Clearing flags on attachment: 376443

Committed r248751: <https://trac.webkit.org/changeset/248751>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54370494>

[Yusuke Suzuki]

Committed r248763: <https://trac.webkit.org/changeset/248763>

[Yusuke Suzuki]

Committed r248764: <https://trac.webkit.org/changeset/248764>