| Summary: | Use one VM per thread for IDB serialization work in network process | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sihui Liu <sihui_liu> | ||||||||||||||||||||
| Component: | New Bugs | Assignee: | Sihui Liu <sihui_liu> | ||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||
| Severity: | Normal | CC: | alecflett, beidson, commit-queue, ews-watchlist, ggaren, jsbell, keith_miller, mark.lam, msaboff, saam, sam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=210731 | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Sihui Liu
2019-08-07 18:03:25 PDT
Created attachment 375774 [details]
Patch
*** Bug 192270 has been marked as a duplicate of this bug. *** Created attachment 375775 [details]
Patch
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 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 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 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? 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 ] Created attachment 376201 [details]
Patch
Created attachment 376211 [details]
Patch
(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 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 (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)? Created attachment 376386 [details]
Test file
(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. Created attachment 376394 [details]
Patch
(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. > > 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 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. Created attachment 376441 [details]
Patch for landing
Created attachment 376443 [details]
Patch for landing
Comment on attachment 376443 [details] Patch for landing Clearing flags on attachment: 376443 Committed r248751: <https://trac.webkit.org/changeset/248751> All reviewed patches have been landed. Closing bug. Committed r248763: <https://trac.webkit.org/changeset/248763> Committed r248764: <https://trac.webkit.org/changeset/248764> |