Attachment 341536 Details for Bug 186064 – patch

[patch] patch

b-backup.diff (text/plain), 5.74 KB, created by Saam Barati on 2018-05-29 16:38:05 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2018-05-29 16:38:05 PDT

Size: 5.74 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 232277)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,42 @@
>+2018-05-29  Saam Barati  <sbarati@apple.com>
>+
>+        Remove JSVirtualMachine shrinkFootprint when clients move to shrinkFootprintWhenIdle
>+        https://bugs.webkit.org/show_bug.cgi?id=186071
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        shrinkFootprint was implemented as:
>+        ```
>+        sanitizeStackForVM(this);
>+        deleteAllCode(DeleteAllCodeIfNotCollecting);
>+        heap.collectNow(Synchronousness::Sync);
>+        WTF::releaseFastMallocFreeMemory();
>+        ```
>+        
>+        However, for correctness reasons, deleteAllCode is implemented to do
>+        work when the VM is idle: no JS is running on the stack. This means
>+        that if shrinkFootprint is called when JS is running on the stack, it
>+        ends up freeing less memory than it could have if it waited to run until
>+        the VM goes idle.
>+        
>+        This patch makes it so we wait until idle before doing work. I'm seeing a
>+        10% footprint progression when testing this against a client of the JSC SPI.
>+        
>+        Because this is a semantic change in how the SPI works, this patch
>+        adds new SPI named shrinkFootprintWhenIdle. The plan is to move
>+        all clients of the shrinkFootprint SPI to shrinkFootprintWhenIdle.
>+        Once that happens, we will delete shrinkFootprint. Until then,
>+        we make shrinkFootprint do exactly what shrinkFootprintWhenIdle does.
>+
>+        * API/JSVirtualMachine.mm:
>+        (-[JSVirtualMachine shrinkFootprint]):
>+        (-[JSVirtualMachine shrinkFootprintWhenIdle]):
>+        * API/JSVirtualMachinePrivate.h:
>+        * runtime/VM.cpp:
>+        (JSC::VM::shrinkFootprintWhenIdle):
>+        (JSC::VM::shrinkFootprint): Deleted.
>+        * runtime/VM.h:
>+
> 2018-05-29  Caio Lima  <ticaiolima@gmail.com>
> 
>         [ESNext][BigInt] Implement support for "<" and ">" relational operation
>Index: Source/JavaScriptCore/API/JSVirtualMachine.mm
>===================================================================
>--- Source/JavaScriptCore/API/JSVirtualMachine.mm	(revision 232275)
>+++ Source/JavaScriptCore/API/JSVirtualMachine.mm	(working copy)
>@@ -272,7 +272,14 @@ - (void)shrinkFootprint
> {
>     JSC::VM* vm = toJS(m_group);
>     JSC::JSLockHolder locker(vm);
>-    vm->shrinkFootprint();
>+    vm->shrinkFootprintWhenIdle();
>+}
>+
>+- (void)shrinkFootprintWhenIdle
>+{
>+    JSC::VM* vm = toJS(m_group);
>+    JSC::JSLockHolder locker(vm);
>+    vm->shrinkFootprintWhenIdle();
> }
> 
> @end
>Index: Source/JavaScriptCore/API/JSVirtualMachinePrivate.h
>===================================================================
>--- Source/JavaScriptCore/API/JSVirtualMachinePrivate.h	(revision 232275)
>+++ Source/JavaScriptCore/API/JSVirtualMachinePrivate.h	(working copy)
>@@ -31,14 +31,20 @@
> 
> @interface JSVirtualMachine(JSPrivate)
> 
>+- (void)shrinkFootprint; // FIXME: Remove this SPI when clients move to shrinkFootprintWhenIdle: https://bugs.webkit.org/show_bug.cgi?id=186071
>+
> /*!
> @method
> @discussion Shrinks the memory footprint of the VM by deleting various internal caches,
>- running synchronous garbage collection, and releasing memory back to the OS. For this
>- to free as much memory as possible, do not call this when JavaScript is running on the stack.
>+ running synchronous garbage collection, and releasing memory back to the OS. Note: this
>+ API waits until no JavaScript is running on the stack before it frees any memory. It's
>+ best to call this API when no JavaScript is running on the stack for this reason. However, if
>+ you do call this API when JavaScript is running on the stack, the API will wait until all JavaScript
>+ on the stack finishes running to free memory back to the OS. Therefore, calling this
>+ API may not synchronously free memory.
> */
> 
>-- (void)shrinkFootprint; // FIXME: Annotate this with NS_AVAILABLE: <rdar://problem/40071332>.
>+- (void)shrinkFootprintWhenIdle; // FIXME: Annotate this with NS_AVAILABLE: <rdar://problem/40071332>.
> 
> @end
> 
>Index: Source/JavaScriptCore/runtime/VM.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/VM.cpp	(revision 232275)
>+++ Source/JavaScriptCore/runtime/VM.cpp	(working copy)
>@@ -777,14 +777,16 @@ void VM::deleteAllCode(DeleteAllCodeEffo
>     });
> }
> 
>-void VM::shrinkFootprint()
>+void VM::shrinkFootprintWhenIdle()
> {
>-    sanitizeStackForVM(this);
>-    deleteAllCode(DeleteAllCodeIfNotCollecting);
>-    heap.collectNow(Synchronousness::Sync);
>-    WTF::releaseFastMallocFreeMemory();
>-    // FIXME: Consider stopping various automatic threads here.
>-    // https://bugs.webkit.org/show_bug.cgi?id=185447
>+    whenIdle([=] () {
>+        sanitizeStackForVM(this);
>+        deleteAllCode(DeleteAllCodeIfNotCollecting);
>+        heap.collectNow(Synchronousness::Sync);
>+        // FIXME: Consider stopping various automatic threads here.
>+        // https://bugs.webkit.org/show_bug.cgi?id=185447
>+        WTF::releaseFastMallocFreeMemory();
>+    });
> }
> 
> SourceProviderCache* VM::addSourceProviderCache(SourceProvider* sourceProvider)
>Index: Source/JavaScriptCore/runtime/VM.h
>===================================================================
>--- Source/JavaScriptCore/runtime/VM.h	(revision 232275)
>+++ Source/JavaScriptCore/runtime/VM.h	(working copy)
>@@ -745,7 +745,7 @@ public:
>     JS_EXPORT_PRIVATE void deleteAllCode(DeleteAllCodeEffort);
>     JS_EXPORT_PRIVATE void deleteAllLinkedCode(DeleteAllCodeEffort);
> 
>-    void shrinkFootprint();
>+    void shrinkFootprintWhenIdle();
> 
>     WatchpointSet* ensureWatchpointSetForImpureProperty(const Identifier&);
>     void registerWatchpointForImpureProperty(const Identifier&, Watchpoint*);

Flags:

mark.lam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 186064: 341529 | 341536 | 341548