Bug 185441 – Add JSVirtualMachine SPI to shrink the memory footprint of the VM

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

<rdar://problem/39999414>

[Saam Barati]

Created attachment 339881 [details]
patch

[EWS Watchlist]

Attachment 339881 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSVirtualMachinePrivate.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Comment on attachment 339881 [details]
patch

r=me.

[Keith Miller]

Comment on attachment 339881 [details]
patch

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

> Source/JavaScriptCore/API/JSVirtualMachinePrivate.h:36
> +@property

I think this should be @method.

[Saam Barati]

Created attachment 339884 [details]
patch for landing

[Saam Barati]

Comment on attachment 339881 [details]
patch

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

>> Source/JavaScriptCore/API/JSVirtualMachinePrivate.h:36
>> +@property
> 
> I think this should be @method.

fixed

[EWS Watchlist]

Attachment 339884 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/API/JSVirtualMachinePrivate.h:26:  Use #pragma once instead of #ifndef for header guard.  [build/header_guard] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 339884 [details]
patch for landing

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

> Source/JavaScriptCore/API/JSVirtualMachinePrivate.h:27
> +#ifndef JSVirtualMachinePrivate_h
> +#define JSVirtualMachinePrivate_h

API/JSVirtualMachine.h doesn't bother with header ifdefs. Should we bother with them here? ObjC is expected to #import which takes care of avoiding duplicate inclusions.

> Source/JavaScriptCore/API/JSVirtualMachinePrivate.h:33
> +@interface JSVirtualMachine(Private)

I think we normally prefix category names like we prefix classes, since all of this ObjC stuff is in the global namespace.

For example WebKit does this with its `WK` prefix:

    @interface WKUserScript (WKPrivate)
    ...
    @end

I think we should do the same. For example:

    @interface JSVirtualMachine (JSPrivate)
    ...
    @end

> Source/JavaScriptCore/runtime/VM.cpp:776
> +    deleteAllCode(DeleteAllCodeIfNotCollecting);

Something we do in Web Inspector when triggering a GC is:

    sanitizeStackForVM(vm);

Is that worth doing here, or maybe it is now unnecessary for Web Inspector?

[Saam Barati]

Comment on attachment 339884 [details]
patch for landing

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

>> Source/JavaScriptCore/API/JSVirtualMachinePrivate.h:27
>> +#define JSVirtualMachinePrivate_h
> 
> API/JSVirtualMachine.h doesn't bother with header ifdefs. Should we bother with them here? ObjC is expected to #import which takes care of avoiding duplicate inclusions.

I think we can get rid of this. JSContext does this so I just copied what it did. But I will remove it. This seems extraneous.

>> Source/JavaScriptCore/runtime/VM.cpp:776
>> +    deleteAllCode(DeleteAllCodeIfNotCollecting);
> 
> Something we do in Web Inspector when triggering a GC is:
> 
>     sanitizeStackForVM(vm);
> 
> Is that worth doing here, or maybe it is now unnecessary for Web Inspector?

No, this is a good idea. I'll do that.

[Saam Barati]

Comment on attachment 339884 [details]
patch for landing

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

>> Source/JavaScriptCore/API/JSVirtualMachinePrivate.h:33
>> +@interface JSVirtualMachine(Private)
> 
> I think we normally prefix category names like we prefix classes, since all of this ObjC stuff is in the global namespace.
> 
> For example WebKit does this with its `WK` prefix:
> 
>     @interface WKUserScript (WKPrivate)
>     ...
>     @end
> 
> I think we should do the same. For example:
> 
>     @interface JSVirtualMachine (JSPrivate)
>     ...
>     @end

This seems reasonable. I'll do this.

[Saam Barati]

Created attachment 339912 [details]
patch for landing

[EWS Watchlist]

Comment on attachment 339912 [details]
patch for landing

Attachment 339912 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/7618974

New failing tests:
apiTests

[EWS Watchlist]

Comment on attachment 339912 [details]
patch for landing

Attachment 339912 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7623493

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 339968 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Saam Barati]

I spoke to Ryan. We're going to land this then monitor JSC api tests to see if they fail on the bots/EWS going forward or not. I can't reproduce api test failure locally.

[Saam Barati]

landed in:
https://trac.webkit.org/changeset/231589/webkit

[Filip Pizlo]

Comment on attachment 339912 [details]
patch for landing

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

> Source/JavaScriptCore/API/tests/testapi.mm:1501
> +    @autoreleasepool {
> +        JSContext *context = [[JSContext alloc] init];
> +        JSVirtualMachine *vm = [context virtualMachine];
> +        [vm shrinkFootprint]; // Make sure that when we allocate a ton of memory below we reuse at little as possible.
> +
> +        std::optional<size_t> footprintBefore = WTF::memoryFootprint();
> +        RELEASE_ASSERT(footprintBefore);
> +
> +        [context evaluateScript:@"for (let i = 0; i < 10000; ++i) { eval(`myVariable_${i} = [i]`); }"];
> +
> +        static constexpr size_t approximateBytes = 10000 * sizeof(int);
> +        std::optional<size_t> footprintMiddle = WTF::memoryFootprint();
> +        RELEASE_ASSERT(footprintMiddle);
> +        checkResult(@"Footprint is larger than what we allocated", *footprintMiddle > approximateBytes);
> +        checkResult(@"Footprint got larger as we allocated a ton of stuff", *footprintMiddle > *footprintBefore);
> +        size_t allocationDelta = *footprintMiddle - *footprintBefore;
> +        checkResult(@"We allocated as much or more memory than what we expected to", allocationDelta >= approximateBytes);
> +
> +        [context evaluateScript:@"for (let i = 0; i < 10000; ++i) { eval(`myVariable_${i} = null`); }"];
> +        [vm shrinkFootprint];
> +        std::optional<size_t> footprintAfter = WTF::memoryFootprint();
> +        RELEASE_ASSERT(footprintAfter);
> +        checkResult(@"Footprint got smaller after we shrank the VM", *footprintAfter < *footprintMiddle);
> +        size_t freeDelta = *footprintMiddle - *footprintAfter;
> +        checkResult(@"Shrinking the footprint of the VM actually freed memory", freeDelta > (approximateBytes / 2));
> +    }
> +

This test just randomly failed on my machine.

I think it's a bad idea to create tests like these.  Footprint is a complex OS metric that can grow or shrink for various reasons.  I don't think it's correct to assert anything about it in a correctness test.

If you want to add perf tests that report memory, and track regressions on those like perf regressions, then that makes sense.  But asserting about footprint is as incorrect as asserting about time.

[Saam Barati]

(In reply to Filip Pizlo from comment #18)
> Comment on attachment 339912 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339912&action=review
> 
> > Source/JavaScriptCore/API/tests/testapi.mm:1501
> > +    @autoreleasepool {
> > +        JSContext *context = [[JSContext alloc] init];
> > +        JSVirtualMachine *vm = [context virtualMachine];
> > +        [vm shrinkFootprint]; // Make sure that when we allocate a ton of memory below we reuse at little as possible.
> > +
> > +        std::optional<size_t> footprintBefore = WTF::memoryFootprint();
> > +        RELEASE_ASSERT(footprintBefore);
> > +
> > +        [context evaluateScript:@"for (let i = 0; i < 10000; ++i) { eval(`myVariable_${i} = [i]`); }"];
> > +
> > +        static constexpr size_t approximateBytes = 10000 * sizeof(int);
> > +        std::optional<size_t> footprintMiddle = WTF::memoryFootprint();
> > +        RELEASE_ASSERT(footprintMiddle);
> > +        checkResult(@"Footprint is larger than what we allocated", *footprintMiddle > approximateBytes);
> > +        checkResult(@"Footprint got larger as we allocated a ton of stuff", *footprintMiddle > *footprintBefore);
> > +        size_t allocationDelta = *footprintMiddle - *footprintBefore;
> > +        checkResult(@"We allocated as much or more memory than what we expected to", allocationDelta >= approximateBytes);
> > +
> > +        [context evaluateScript:@"for (let i = 0; i < 10000; ++i) { eval(`myVariable_${i} = null`); }"];
> > +        [vm shrinkFootprint];
> > +        std::optional<size_t> footprintAfter = WTF::memoryFootprint();
> > +        RELEASE_ASSERT(footprintAfter);
> > +        checkResult(@"Footprint got smaller after we shrank the VM", *footprintAfter < *footprintMiddle);
> > +        size_t freeDelta = *footprintMiddle - *footprintAfter;
> > +        checkResult(@"Shrinking the footprint of the VM actually freed memory", freeDelta > (approximateBytes / 2));
> > +    }
> > +
> 
> This test just randomly failed on my machine.
> 
> I think it's a bad idea to create tests like these.  Footprint is a complex
> OS metric that can grow or shrink for various reasons.  I don't think it's
> correct to assert anything about it in a correctness test.
> 
> If you want to add perf tests that report memory, and track regressions on
> those like perf regressions, then that makes sense.  But asserting about
> footprint is as incorrect as asserting about time.

Yeah I agree with this w.r.t the OS’s footprint. However, I think we should be able to have this test but with our own internal calculation of the VM’s heap size. What do you think about converting it to be that?