Bug 200526 - Use one VM per thread for IDB serialization work in network process
Summary: Use one VM per thread for IDB serialization work in network process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
: 192270 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-08-07 18:03 PDT by Sihui Liu
Modified: 2020-04-19 22:45 PDT (History)
14 users (show)

See Also:


Attachments
Patch (24.64 KB, patch)
2019-08-07 18:16 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (24.17 KB, patch)
2019-08-07 18:22 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-highsierra (3.55 MB, application/zip)
2019-08-07 19:55 PDT, EWS Watchlist
no flags Details
Patch (33.21 KB, patch)
2019-08-13 12:39 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (27.68 KB, patch)
2019-08-13 14:05 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Test file (1.31 KB, text/html)
2019-08-15 09:29 PDT, Sihui Liu
no flags Details
Patch (26.37 KB, patch)
2019-08-15 11:00 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (30.66 KB, patch)
2019-08-15 16:40 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (30.65 KB, patch)
2019-08-15 16:42 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2019-08-07 18:03:25 PDT
Probably a memory improvement.
Comment 1 Sihui Liu 2019-08-07 18:16:13 PDT
Created attachment 375774 [details]
Patch
Comment 2 Sihui Liu 2019-08-07 18:17:58 PDT
*** Bug 192270 has been marked as a duplicate of this bug. ***
Comment 3 Sihui Liu 2019-08-07 18:22:58 PDT
Created attachment 375775 [details]
Patch
Comment 4 EWS Watchlist 2019-08-07 19:55:29 PDT
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
Comment 5 EWS Watchlist 2019-08-07 19:55:31 PDT
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
Comment 6 Sam Weinig 2019-08-07 22:26:05 PDT
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?
Comment 7 Sam Weinig 2019-08-07 22:26:07 PDT
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?
Comment 8 Geoffrey Garen 2019-08-08 12:45:29 PDT
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 ]
Comment 9 Sihui Liu 2019-08-13 12:39:59 PDT
Created attachment 376201 [details]
Patch
Comment 10 Sihui Liu 2019-08-13 14:05:03 PDT
Created attachment 376211 [details]
Patch
Comment 11 Sihui Liu 2019-08-13 14:18:35 PDT
(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.
Comment 12 EWS Watchlist 2019-08-13 16:22:04 PDT
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
Comment 13 Sam Weinig 2019-08-13 20:12:32 PDT
(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)?
Comment 14 Sihui Liu 2019-08-15 09:29:20 PDT
Created attachment 376386 [details]
Test file
Comment 15 Sihui Liu 2019-08-15 09:38:47 PDT
(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.
Comment 16 Sihui Liu 2019-08-15 11:00:25 PDT
Created attachment 376394 [details]
Patch
Comment 17 Geoffrey Garen 2019-08-15 13:43:17 PDT
(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.
Comment 18 Geoffrey Garen 2019-08-15 13:54:08 PDT
> > 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.
Comment 19 Geoffrey Garen 2019-08-15 14:02:14 PDT
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.
Comment 20 Sihui Liu 2019-08-15 16:40:38 PDT
Created attachment 376441 [details]
Patch for landing
Comment 21 Sihui Liu 2019-08-15 16:42:16 PDT
Created attachment 376443 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2019-08-15 17:12:48 PDT
Comment on attachment 376443 [details]
Patch for landing

Clearing flags on attachment: 376443

Committed r248751: <https://trac.webkit.org/changeset/248751>
Comment 23 WebKit Commit Bot 2019-08-15 17:12:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Radar WebKit Bug Importer 2019-08-15 17:13:36 PDT
<rdar://problem/54370494>
Comment 25 Yusuke Suzuki 2019-08-16 00:46:13 PDT
Committed r248763: <https://trac.webkit.org/changeset/248763>
Comment 26 Yusuke Suzuki 2019-08-16 00:54:49 PDT
Committed r248764: <https://trac.webkit.org/changeset/248764>