⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 242980 in webkit


Ignore:
Timestamp:
Mar 14, 2019, 6:10:41 PM (7 years ago)
Author:
sbarati@apple.com
Message:

Remove retain cycle from JSScript and also don't keep the cache file descriptor open so many JSScripts can be cached in a loop
​https://bugs.webkit.org/show_bug.cgi?id=195782
<rdar://problem/48880625>

Reviewed by Michael Saboff.

This patch fixes two issues with JSScript API:

  1. There was a retain cycle causing us to never destroy a JSScript once it

created a JSSourceCode. The reason for this is that JSScript had a
Strong<JSSourceCode> field. And JSSourceCode transitively had RetainPtr<JSScript>.

This patch fixes this issue by making the "jsSourceCode" accessor return a transient object.

  1. r242585 made it so that JSScript would keep the cache file descriptor open

(and locked) for the duration of the lifetime of the JSScript itself. Our
anticipation here is that it would make implementing iterative cache updates
easier. However, this made using the API super limiting in other ways. For
example, if a program had a loop that cached 3000 different JSScripts, it's
likely that such a program would exhaust the open file count limit. This patch
reverts to the behavior prior to r242585 where we just keep open the file descriptor
while we read or write it.

  • API/JSAPIGlobalObject.mm:

(JSC::JSAPIGlobalObject::moduleLoaderFetch):

  • API/JSContext.mm:

(-[JSContext evaluateJSScript:]):

  • API/JSScript.mm:

(-[JSScript dealloc]):
(-[JSScript readCache]):
(-[JSScript init]):
(-[JSScript sourceCode]):
(-[JSScript jsSourceCode]):
(-[JSScript writeCache:]):
(-[JSScript forceRecreateJSSourceCode]): Deleted.

  • API/JSScriptInternal.h:
  • API/tests/testapi.mm:

(testCanCacheManyFilesWithTheSameVM):
(testObjectiveCAPI):
(testCacheFileIsExclusive): Deleted.

Location:
trunk/Source/JavaScriptCore
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/API/JSAPIGlobalObject.mm

    r241929 r242980  
    205205        } else {
    206206            [jsScript setSourceURL:[NSURL URLWithString:static_cast<NSString *>(moduleKey.string())]];
    207             source = [jsScript forceRecreateJSSourceCode];
     207            source = [jsScript jsSourceCode];
    208208        }
    209209

  • trunk/Source/JavaScriptCore/API/JSContext.mm

    r241929 r242980  
    125125    if (script.type == kJSScriptTypeProgram) {
    126126        JSValueRef exceptionValue = nullptr;
    127         JSValueRef result = JSEvaluateScriptInternal(locker, exec, m_context, nullptr, [script jsSourceCode]->sourceCode(), &exceptionValue);
     127        JSC::SourceCode sourceCode = [script sourceCode];
     128        JSValueRef result = JSEvaluateScriptInternal(locker, exec, m_context, nullptr, sourceCode, &exceptionValue);
    128129
    129130        if (exceptionValue)

  • trunk/Source/JavaScriptCore/API/JSScript.mm

    r242585 r242980  
    4040#include <sys/stat.h>
    4141#include <wtf/FileSystem.h>
     42#include <wtf/Scope.h>
    4243
    4344#if JSC_OBJC_API_ENABLED
    … …  
    5152    RetainPtr<NSURL> m_cachePath;
    5253    JSC::CachedBytecode m_cachedBytecode;
    53     JSC::Strong<JSC::JSSourceCode> m_jsSourceCode;
    54     int m_cacheFileDescriptor;
    5554}
    5655
    … …  
    176175        munmap(const_cast<void*>(m_cachedBytecode.data()), m_cachedBytecode.size());
    177176
    178     if (m_cacheFileDescriptor != -1)
    179         close(m_cacheFileDescriptor);
    180 
    181177    [super dealloc];
    182178}
    … …  
    187183        return;
    188184
    189     m_cacheFileDescriptor = open([m_cachePath path].UTF8String, O_CREAT | O_RDWR | O_EXLOCK | O_NONBLOCK, 0666);
    190     if (m_cacheFileDescriptor == -1)
     185    int fd = open([m_cachePath path].UTF8String, O_RDONLY | O_EXLOCK | O_NONBLOCK, 0666);
     186    if (fd == -1)
    191187        return;
     188    auto closeFD = makeScopeExit([&] {
     189        close(fd);
     190    });
    192191
    193192    struct stat sb;
    194     int res = fstat(m_cacheFileDescriptor, &sb);
     193    int res = fstat(fd, &sb);
    195194    size_t size = static_cast<size_t>(sb.st_size);
    196195    if (res || !size)
    197196        return;
    198197
    199     void* buffer = mmap(nullptr, size, PROT_READ, MAP_PRIVATE, m_cacheFileDescriptor, 0);
     198    void* buffer = mmap(nullptr, size, PROT_READ, MAP_PRIVATE, fd, 0);
    200199
    201200    JSC::CachedBytecode cachedBytecode { buffer, size };
    202201
    203202    JSC::VM& vm = m_virtualMachine.vm;
    204     const JSC::SourceCode& sourceCode = [self jsSourceCode]->sourceCode();
     203    JSC::SourceCode sourceCode = [self sourceCode];
    205204    JSC::SourceCodeKey key = m_type == kJSScriptTypeProgram ? sourceCodeKeyForSerializedProgram(vm, sourceCode) : sourceCodeKeyForSerializedModule(vm, sourceCode);
    206205    if (isCachedBytecodeStillValid(vm, cachedBytecode, key, m_type == kJSScriptTypeProgram ? JSC::SourceCodeType::ProgramType : JSC::SourceCodeType::ModuleType))
    207206        m_cachedBytecode = WTFMove(cachedBytecode);
    208207    else
    209         ftruncate(m_cacheFileDescriptor, 0);
     208        ftruncate(fd, 0);
    210209}
    211210
    … …  
    242241        return nil;
    243242
    244     m_cacheFileDescriptor = -1;
    245243    return self;
    246244}
    … …  
    261259}
    262260
    263 - (JSC::JSSourceCode*)jsSourceCode
    264 {
    265     if (m_jsSourceCode)
    266         return m_jsSourceCode.get();
    267 
    268     return [self forceRecreateJSSourceCode];
    269 }
    270 
    271 - (BOOL)writeCache:(String&)error
    272 {
    273     if (m_cachedBytecode.size()) {
    274         error = "Cache for JSScript is already non-empty. Can not override it."_s;
    275         return NO;
    276     }
    277 
    278     if (m_cacheFileDescriptor == -1) {
    279         if (!m_cachePath)
    280             error = "No cache was path provided during construction of this JSScript."_s;
    281         else
    282             error = "Could not lock the bytecode cache file. It's likely another VM or process is already using it."_s;
    283         return NO;
    284     }
    285 
    286     JSC::ParserError parserError;
    287     switch (m_type) {
    288     case kJSScriptTypeModule:
    289         m_cachedBytecode = JSC::generateModuleBytecode(m_virtualMachine.vm, [self jsSourceCode]->sourceCode(), parserError);
    290         break;
    291     case kJSScriptTypeProgram:
    292         m_cachedBytecode = JSC::generateProgramBytecode(m_virtualMachine.vm, [self jsSourceCode]->sourceCode(), parserError);
    293         break;
    294     }
    295 
    296     if (parserError.isValid()) {
    297         m_cachedBytecode = { };
    298         error = makeString("Unable to generate bytecode for this JSScript because of a parser error: ", parserError.message());
    299         return NO;
    300     }
    301 
    302     ssize_t bytesWritten = write(m_cacheFileDescriptor, m_cachedBytecode.data(), m_cachedBytecode.size());
    303     if (bytesWritten == -1) {
    304         error = makeString("Could not write cache file to disk: ", strerror(errno));
    305         return NO;
    306     }
    307 
    308     if (static_cast<size_t>(bytesWritten) != m_cachedBytecode.size()) {
    309         ftruncate(m_cacheFileDescriptor, 0);
    310         error = makeString("Could not write the full cache file to disk. Only wrote ", String::number(bytesWritten), " of the expected ", String::number(m_cachedBytecode.size()), " bytes.");
    311         return NO;
    312     }
    313 
    314     return YES;
    315 }
    316 
    317 - (void)setSourceURL:(NSURL *)url
    318 {
    319     m_sourceURL = url;
    320 }
    321 
    322 - (JSC::JSSourceCode*)forceRecreateJSSourceCode
     261- (JSC::SourceCode)sourceCode
    323262{
    324263    JSC::VM& vm = m_virtualMachine.vm;
    … …  
    326265
    327266    TextPosition startPosition { };
    328 
    329267    String url = String { [[self sourceURL] absoluteString] };
    330268    auto type = m_type == kJSScriptTypeModule ? JSC::SourceProviderSourceType::Module : JSC::SourceProviderSourceType::Program;
    331269    Ref<JSScriptSourceProvider> sourceProvider = JSScriptSourceProvider::create(self, JSC::SourceOrigin(url), URL({ }, url), startPosition, type);
    332270    JSC::SourceCode sourceCode(WTFMove(sourceProvider), startPosition.m_line.oneBasedInt(), startPosition.m_column.oneBasedInt());
    333     JSC::JSSourceCode* jsSourceCode = JSC::JSSourceCode::create(vm, WTFMove(sourceCode));
    334     m_jsSourceCode.set(vm, jsSourceCode);
     271    return sourceCode;
     272}
     273
     274- (JSC::JSSourceCode*)jsSourceCode
     275{
     276    JSC::VM& vm = m_virtualMachine.vm;
     277    JSC::JSLockHolder locker(vm);
     278    JSC::JSSourceCode* jsSourceCode = JSC::JSSourceCode::create(vm, [self sourceCode]);
    335279    return jsSourceCode;
    336280}
    337281
     282- (BOOL)writeCache:(String&)error
     283{
     284    if (m_cachedBytecode.size()) {
     285        error = "Cache for JSScript is already non-empty. Can not override it."_s;
     286        return NO;
     287    }
     288
     289    if (!m_cachePath) {
     290        error = "No cache path was provided during construction of this JSScript."_s;
     291        return NO;
     292    }
     293
     294    int fd = open([m_cachePath path].UTF8String, O_CREAT | O_RDWR | O_EXLOCK | O_NONBLOCK, 0666);
     295    if (fd == -1) {
     296        error = makeString("Could not open or lock the bytecode cache file. It's likely another VM or process is already using it. Error: ", strerror(errno));
     297        return NO;
     298    }
     299    auto closeFD = makeScopeExit([&] {
     300        close(fd);
     301    });
     302
     303    JSC::ParserError parserError;
     304    JSC::SourceCode sourceCode = [self sourceCode];
     305    switch (m_type) {
     306    case kJSScriptTypeModule:
     307        m_cachedBytecode = JSC::generateModuleBytecode(m_virtualMachine.vm, sourceCode, parserError);
     308        break;
     309    case kJSScriptTypeProgram:
     310        m_cachedBytecode = JSC::generateProgramBytecode(m_virtualMachine.vm, sourceCode, parserError);
     311        break;
     312    }
     313
     314    if (parserError.isValid()) {
     315        m_cachedBytecode = { };
     316        error = makeString("Unable to generate bytecode for this JSScript because of a parser error: ", parserError.message());
     317        return NO;
     318    }
     319
     320    ssize_t bytesWritten = write(fd, m_cachedBytecode.data(), m_cachedBytecode.size());
     321    if (bytesWritten == -1) {
     322        error = makeString("Could not write cache file to disk: ", strerror(errno));
     323        return NO;
     324    }
     325
     326    if (static_cast<size_t>(bytesWritten) != m_cachedBytecode.size()) {
     327        ftruncate(fd, 0);
     328        error = makeString("Could not write the full cache file to disk. Only wrote ", String::number(bytesWritten), " of the expected ", String::number(m_cachedBytecode.size()), " bytes.");
     329        return NO;
     330    }
     331
     332    return YES;
     333}
     334
     335- (void)setSourceURL:(NSURL *)url
     336{
     337    m_sourceURL = url;
     338}
     339
    338340@end
    339341

  • trunk/Source/JavaScriptCore/API/JSScriptInternal.h

    r242585 r242980  
    5050- (nullable const JSC::CachedBytecode*)cachedBytecode;
    5151- (JSC::JSSourceCode*)jsSourceCode;
     52- (JSC::SourceCode)sourceCode;
     53- (BOOL)writeCache:(String&)error;
    5254// FIXME: Remove this once we require sourceURL upon creation: https://bugs.webkit.org/show_bug.cgi?id=194909
    53 - (JSC::JSSourceCode*)forceRecreateJSSourceCode;
    54 - (BOOL)writeCache:(String&)error;
    5555- (void)setSourceURL:(NSURL *)url;
    5656

  • trunk/Source/JavaScriptCore/API/tests/testapi.mm

    r242585 r242980  
    21602160}
    21612161
    2162 static void testCacheFileIsExclusive()
    2163 {
    2164     NSURL* cachePath = tempFile(@"foo.program.cache");
    2165 
    2166     @autoreleasepool {
    2167         NSString *source = @"function foo() { return 42; } foo();";
    2168         NSURL* sourceURL = [NSURL URLWithString:@"my-path"];
    2169         JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
    2170 
    2171         JSScript *script1 = [JSScript scriptOfType:kJSScriptTypeProgram withSource:source andSourceURL:sourceURL andBytecodeCache:cachePath inVirtualMachine:vm error:nil];
    2172         RELEASE_ASSERT(script1);
    2173         checkResult(@"Should be able to cache the first file", [script1 cacheBytecodeWithError:nil]);
    2174 
    2175         JSScript *script2 = [JSScript scriptOfType:kJSScriptTypeProgram withSource:source andSourceURL:sourceURL andBytecodeCache:cachePath inVirtualMachine:vm error:nil];
    2176         RELEASE_ASSERT(script2);
    2177         NSError* error = nil;
    2178         checkResult(@"Should NOT be able to cache the second file", ![script2 cacheBytecodeWithError:&error]);
    2179         checkResult(@"Should NOT be able to cache the second file has the correct error message", [[error description] containsString:@"Could not lock the bytecode cache file. It's likely another VM or process is already using it"]);
    2180     }
    2181 
    2182     NSFileManager* fileManager = [NSFileManager defaultManager];
    2183     BOOL removedAll = [fileManager removeItemAtURL:cachePath error:nil];
    2184     checkResult(@"Successfully removed cache file", removedAll);
    2185 }
    2186 
    21872162static void testCacheFileFailsWhenItsAlreadyCached()
    21882163{
    … …  
    22192194    BOOL removedAll = [fileManager removeItemAtURL:cachePath error:nil];
    22202195    checkResult(@"Successfully removed cache file", removedAll);
     2196}
     2197
     2198static void testCanCacheManyFilesWithTheSameVM()
     2199{
     2200    NSMutableArray *cachePaths = [[NSMutableArray alloc] init];
     2201    NSMutableArray *scripts = [[NSMutableArray alloc] init];
     2202
     2203    for (unsigned i = 0; i < 10000; ++i)
     2204        [cachePaths addObject:tempFile([NSString stringWithFormat:@"cache-%d.cache", i])];
     2205
     2206    JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
     2207    bool cachedAll = true;
     2208    for (NSURL *path : cachePaths) {
     2209        @autoreleasepool {
     2210            NSURL *sourceURL = [NSURL URLWithString:@"id"];
     2211            NSString *source = @"function foo() { return 42; } foo();";
     2212            JSScript *script = [JSScript scriptOfType:kJSScriptTypeProgram withSource:source andSourceURL:sourceURL andBytecodeCache:path inVirtualMachine:vm error:nil];
     2213            RELEASE_ASSERT(script);
     2214
     2215            [scripts addObject:script];
     2216            cachedAll &= [script cacheBytecodeWithError:nil];
     2217        }
     2218    }
     2219    checkResult(@"Cached all 10000 scripts", cachedAll);
     2220
     2221    JSContext *context = [[JSContext alloc] init];
     2222    bool all42 = true;
     2223    for (JSScript *script : scripts) {
     2224        @autoreleasepool {
     2225            JSValue *result = [context evaluateJSScript:script];
     2226            RELEASE_ASSERT(result);
     2227            all42 &= [result isNumber] && [result toInt32] == 42;
     2228        }
     2229    }
     2230    checkResult(@"All scripts returned 42", all42);
     2231
     2232    NSFileManager* fileManager = [NSFileManager defaultManager];
     2233    bool removedAll = true;
     2234    for (NSURL *path : cachePaths)
     2235        removedAll &= [fileManager removeItemAtURL:path error:nil];
     2236
     2237    checkResult(@"Removed all cache files", removedAll);
    22212238}
    22222239
    … …  
    24282445    RUN(testBytecodeCacheWithSameCacheFileAndDifferentScript(true));
    24292446    RUN(testProgramJSScriptException());
    2430     RUN(testCacheFileIsExclusive());
    24312447    RUN(testCacheFileFailsWhenItsAlreadyCached());
     2448    RUN(testCanCacheManyFilesWithTheSameVM());
    24322449
    24332450    RUN(testLoaderRejectsNilScriptURL());

  • trunk/Source/JavaScriptCore/ChangeLog

    r242955 r242980  
     12019-03-14  Saam barati  <sbarati@apple.com>
     2
     3        Remove retain cycle from JSScript and also don't keep the cache file descriptor open so many JSScripts can be cached in a loop
     4        https://bugs.webkit.org/show_bug.cgi?id=195782
     5        <rdar://problem/48880625>
     6
     7        Reviewed by Michael Saboff.
     8
     9        This patch fixes two issues with JSScript API:
     10       
     11        1. There was a retain cycle causing us to never destroy a JSScript once it
     12        created a JSSourceCode. The reason for this is that JSScript had a
     13        Strong<JSSourceCode> field. And JSSourceCode transitively had RetainPtr<JSScript>.
     14       
     15        This patch fixes this issue by making the "jsSourceCode" accessor return a transient object.
     16       
     17        2. r242585 made it so that JSScript would keep the cache file descriptor open
     18        (and locked) for the duration of the lifetime of the JSScript itself. Our
     19        anticipation here is that it would make implementing iterative cache updates
     20        easier. However, this made using the API super limiting in other ways. For
     21        example, if a program had a loop that cached 3000 different JSScripts, it's
     22        likely that such a program would exhaust the open file count limit. This patch
     23        reverts to the behavior prior to r242585 where we just keep open the file descriptor
     24        while we read or write it.
     25
     26        * API/JSAPIGlobalObject.mm:
     27        (JSC::JSAPIGlobalObject::moduleLoaderFetch):
     28        * API/JSContext.mm:
     29        (-[JSContext evaluateJSScript:]):
     30        * API/JSScript.mm:
     31        (-[JSScript dealloc]):
     32        (-[JSScript readCache]):
     33        (-[JSScript init]):
     34        (-[JSScript sourceCode]):
     35        (-[JSScript jsSourceCode]):
     36        (-[JSScript writeCache:]):
     37        (-[JSScript forceRecreateJSSourceCode]): Deleted.
     38        * API/JSScriptInternal.h:
     39        * API/tests/testapi.mm:
     40        (testCanCacheManyFilesWithTheSameVM):
     41        (testObjectiveCAPI):
     42        (testCacheFileIsExclusive): Deleted.
     43
    1442019-03-14  Michael Saboff  <msaboff@apple.com>
    245
Note: See TracChangeset for help on using the changeset viewer.