...
<rdar://problem/39999414>
Created attachment 339881 [details] patch
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.
Comment on attachment 339881 [details] patch r=me.
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.
Created attachment 339884 [details] patch for landing
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
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.
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?
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.
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.
Created attachment 339912 [details] patch for landing
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
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
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
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.
landed in: https://trac.webkit.org/changeset/231589/webkit
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.
(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?