Bytecode cache should use FileSystem

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Christopher Reid]

It currently uses posix file and memory mapping calls.

[Christopher Reid]

Created attachment 374039 [details]
patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/jsc.cpp:1077:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/FileSystem.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/FileSystem.h:204:  The parameter name "handle" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/FileSystem.h:215:  The parameter name "handle" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 23 files


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

[Christopher Reid]

Created attachment 374040 [details]
Patch

style fixes

[Tadeu Zagallo]

Comment on attachment 374040 [details]
Patch

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

The cache changes look great! I'll let someone else review the glib and windows code.

> Source/JavaScriptCore/jsc.cpp:1077
> +        m_cachedBytecode = CachedBytecode::create(WTFMove(mappedFileData), { });

just a nit, but I don't think you need the second parameter here.

> Source/WTF/wtf/win/FileSystemWin.cpp:640
>      CloseHandle(mapping);

isn't this already handled by the caller now?

[Christopher Reid]

Comment on attachment 374040 [details]
Patch

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

>> Source/JavaScriptCore/jsc.cpp:1077
>> +        m_cachedBytecode = CachedBytecode::create(WTFMove(mappedFileData), { });
> 
> just a nit, but I don't think you need the second parameter here.

Yeah it doesn't seem needed I'll remove it.

>> Source/WTF/wtf/win/FileSystemWin.cpp:640
>>      CloseHandle(mapping);
> 
> isn't this already handled by the caller now?

The caller now handles the close for the file handle passed in but not the handle returned from CreateFileMapping.

[Michael Catanzaro]

Comment on attachment 374040 [details]
Patch

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

> Source/WTF/wtf/FileSystem.cpp:314
> +#if USE(GLIB)
> +    auto* inputStream = g_io_stream_get_input_stream(G_IO_STREAM(handle));
> +    fd = g_file_descriptor_based_get_fd(G_FILE_DESCRIPTOR_BASED(inputStream));
> +#else
> +    fd = handle;
> +#endif

If I wanted to be a Nazi reviewer I could tell you to rewrite all of MappedFileData using GMappedFile.

I'm not that horrible, fortunately! GLib changes r=me.

> Source/WTF/wtf/glib/FileSystemGlib.cpp:398
> +    return g_seekable_truncate(G_SEEKABLE(g_io_stream_get_output_stream(G_IO_STREAM(handle))), offset, 0, 0);

nullptr, nullptr

I'll add a TODO for me to come back and convert the rest of this file to nullptr later.

Normally I like to check for errors and print a warning when the operation fails, but I guess we shouldn't do that here because the other platform implementations don't and that would surely lead to errors that only print for the GLib impl.

[Christopher Reid]

Comment on attachment 374040 [details]
Patch

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

>> Source/WTF/wtf/FileSystem.cpp:314
>> +#endif
> 
> If I wanted to be a Nazi reviewer I could tell you to rewrite all of MappedFileData using GMappedFile.
> 
> I'm not that horrible, fortunately! GLib changes r=me.

I was originally planning on doing that but GMappedFile only does MAP_PRIVATE mappings https://github.com/GNOME/glib/blob/c8692fffe0723aee2bc6710590b5e70ef841b5cc/glib/gmappedfile.c#L160.

[Christopher Reid]

Created attachment 374077 [details]
Updated Patch

[Stephan Szabo]

Comment on attachment 374077 [details]
Updated Patch

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

> Source/WTF/wtf/FileSystem.cpp:294
> +    if (!isHandleValid(fd)) {

Is this really necessary any longer since mapFileHandle returns false for and closeFile should be ignoring PlatformFileHandles that aren't valid?

> Source/WTF/wtf/win/FileSystemWin.cpp:479
> +    eofInfo.EndOfFile = largeOffset;

Might be nicer to just set eof.EndOfFile.QuadPart = offset rather than having the temporary.

> Source/WTF/wtf/win/FileSystemWin.cpp:612
> +    if (file == INVALID_HANDLE_VALUE) {

Like the main FileSystem.cpp, is there a reason to not drop this explicit check and use closeFile at the end?

[Christopher Reid]

(In reply to Stephan Szabo from comment #9)
> Comment on attachment 374077 [details]
> Updated Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374077&action=review
>
> > Source/WTF/wtf/win/FileSystemWin.cpp:479
> > +    eofInfo.EndOfFile = largeOffset;
> 
> Might be nicer to just set eof.EndOfFile.QuadPart = offset rather than
> having the temporary.

Right that temporary isn't needed, I'll take it out.

> > Source/WTF/wtf/win/FileSystemWin.cpp:612
> > +    if (file == INVALID_HANDLE_VALUE) {
> 
> Like the main FileSystem.cpp, is there a reason to not drop this explicit
> check and use closeFile at the end?

They seem safe to drop, I'll take those out.

[Christopher Reid]

Created attachment 374164 [details]
Updated Patch

[Yusuke Suzuki]

Comment on attachment 374164 [details]
Updated Patch

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

r=me with nits.

> Source/JavaScriptCore/API/JSScript.mm:164
> +    FileSystem::MappedFileData mappedFile(fd, FileSystem::MappedFileMode::Private, success);

We should change this interface to something like this instead of passing boolean `success`.

Optional<MappedFileData> mappedFile = FileSystem::MappedFileData::open(...);

But this is not directly related to this patch. So I'm OK with this. Just commented for further refactoring.

> Source/JavaScriptCore/runtime/CachePayload.cpp:82
>  #if !OS(WINDOWS)
>          munmap(m_data, m_size);
>  #else
> -        RELEASE_ASSERT_NOT_REACHED();
> +        UnmapViewOfFile(m_data);
>  #endif
>      } else

Can we extract this part in WTF::FileSystem as `FileSystem::unmapViewOfFile(buffer, size)` (and using it in MappedFileData::~MappedFileData)?

> Source/WTF/wtf/FileSystem.cpp:299
> +bool MappedFileData::mapFileHandle(PlatformFileHandle handle, MappedFileMode mode)

Nice cleanup.

[Christopher Reid]

(In reply to Yusuke Suzuki from comment #12)
> Comment on attachment 374164 [details]
> Updated Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=374164&action=review
> 
> r=me with nits.
> 
> > Source/JavaScriptCore/API/JSScript.mm:164
> > +    FileSystem::MappedFileData mappedFile(fd, FileSystem::MappedFileMode::Private, success);
> 
> We should change this interface to something like this instead of passing
> boolean `success`.
> 
> Optional<MappedFileData> mappedFile = FileSystem::MappedFileData::open(...);
> 
> But this is not directly related to this patch. So I'm OK with this. Just
> commented for further refactoring.
> 

That's a good improvement. I'll look at doing that in a followup patch.

> > Source/JavaScriptCore/runtime/CachePayload.cpp:82
> >  #if !OS(WINDOWS)
> >          munmap(m_data, m_size);
> >  #else
> > -        RELEASE_ASSERT_NOT_REACHED();
> > +        UnmapViewOfFile(m_data);
> >  #endif
> >      } else
> 
> Can we extract this part in WTF::FileSystem as
> `FileSystem::unmapViewOfFile(buffer, size)` (and using it in
> MappedFileData::~MappedFileData)?

Sounds good I'll update that before landing.

[Christopher Reid]

Created attachment 374278 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 374278 [details]
Patch for landing

Clearing flags on attachment: 374278

Committed r247505: <https://trac.webkit.org/changeset/247505>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53188300>

[Ryan Haddad]

(In reply to Radar WebKit Bug Importer from comment #17)
> <rdar://problem/53188300>

stress/tricky-indirectly-inferred-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js.bytecode-cache is failing an assertion on the debug JSC bot after this change:

ASSERTION FAILED: codeBlock
./runtime/CodeCache.h(172) : std::enable_if_t<std::is_base_of<UnlinkedCodeBlock, UnlinkedCodeBlockType>::value && !std::is_same<UnlinkedCodeBlockType, UnlinkedEvalCodeBlock>::value, UnlinkedCodeBlockType *> JSC::CodeCacheMap::fetchFromDisk(JSC::VM &, const JSC::SourceCodeKey &) [UnlinkedCodeBlockType = JSC::UnlinkedProgramCodeBlock]
1   0x108929ba9 WTFCrash
2   0x10892cb8b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x109e4f4fa std::__1::enable_if<(std::is_base_of<JSC::UnlinkedCodeBlock, JSC::UnlinkedProgramCodeBlock>::value) && (!(std::is_same<JSC::UnlinkedProgramCodeBlock, JSC::UnlinkedEvalCodeBlock>::value)), JSC::UnlinkedProgramCodeBlock*>::type JSC::CodeCacheMap::fetchFromDisk<JSC::UnlinkedProgramCodeBlock>(JSC::VM&, JSC::SourceCodeKey const&)
4   0x109e4ef38 JSC::UnlinkedProgramCodeBlock* JSC::CodeCacheMap::findCacheAndUpdateAge<JSC::UnlinkedProgramCodeBlock>(JSC::VM&, JSC::SourceCodeKey const&)
5   0x109e2b4f6 JSC::UnlinkedProgramCodeBlock* JSC::CodeCache::getUnlinkedGlobalCodeBlock<JSC::UnlinkedProgramCodeBlock, JSC::ProgramExecutable>(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserStrictMode, JSC::JSParserScriptMode, WTF::OptionSet<JSC::CodeGenerationMode>, JSC::ParserError&, JSC::EvalContextType)
6   0x109e2b365 JSC::CodeCache::getUnlinkedProgramCodeBlock(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserStrictMode, WTF::OptionSet<JSC::CodeGenerationMode>, JSC::ParserError&)
7   0x10a05d556 JSC::ProgramExecutable::initializeGlobalProperties(JSC::VM&, JSC::ExecState*, JSC::JSScope*)
8   0x109b0b7ac JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
9   0x109e47505 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
10  0x108849f17 runWithOptions(GlobalObject*, CommandLine&, bool&)
11  0x10881f75a jscmain(int, char**)::$_6::operator()(JSC::VM&, GlobalObject*, bool&) const
12  0x1087fe804 int runJSC<jscmain(int, char**)::$_6>(CommandLine const&, bool, jscmain(int, char**)::$_6 const&)
13  0x1087fcf31 jscmain(int, char**)
14  0x1087fcd9e main
15  0x7fff54f22015 start
16  0x8

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/3251

[Christopher Reid]

(In reply to Ryan Haddad from comment #18)
> (In reply to Radar WebKit Bug Importer from comment #17)
> > <rdar://problem/53188300>
> 
> stress/tricky-indirectly-inferred-infinite-loop-that-uses-captured-variables-
> and-creates-the-activation-outside-the-loop.js.bytecode-cache is failing an
> assertion on the debug JSC bot after this change:
> 
> ASSERTION FAILED: codeBlock
> ./runtime/CodeCache.h(172) :
> std::enable_if_t<std::is_base_of<UnlinkedCodeBlock,
> UnlinkedCodeBlockType>::value && !std::is_same<UnlinkedCodeBlockType,
> UnlinkedEvalCodeBlock>::value, UnlinkedCodeBlockType *>
> JSC::CodeCacheMap::fetchFromDisk(JSC::VM &, const JSC::SourceCodeKey &)
> [UnlinkedCodeBlockType = JSC::UnlinkedProgramCodeBlock]
> 1   0x108929ba9 WTFCrash
> 2   0x10892cb8b WTFCrashWithInfo(int, char const*, char const*, int)
> 3   0x109e4f4fa std::__1::enable_if<(std::is_base_of<JSC::UnlinkedCodeBlock,
> JSC::UnlinkedProgramCodeBlock>::value) &&
> (!(std::is_same<JSC::UnlinkedProgramCodeBlock,
> JSC::UnlinkedEvalCodeBlock>::value)), JSC::UnlinkedProgramCodeBlock*>::type
> JSC::CodeCacheMap::fetchFromDisk<JSC::UnlinkedProgramCodeBlock>(JSC::VM&,
> JSC::SourceCodeKey const&)
> 4   0x109e4ef38 JSC::UnlinkedProgramCodeBlock*
> JSC::CodeCacheMap::findCacheAndUpdateAge<JSC::UnlinkedProgramCodeBlock>(JSC::
> VM&, JSC::SourceCodeKey const&)
> 5   0x109e2b4f6 JSC::UnlinkedProgramCodeBlock*
> JSC::CodeCache::getUnlinkedGlobalCodeBlock<JSC::UnlinkedProgramCodeBlock,
> JSC::ProgramExecutable>(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode
> const&, JSC::JSParserStrictMode, JSC::JSParserScriptMode,
> WTF::OptionSet<JSC::CodeGenerationMode>, JSC::ParserError&,
> JSC::EvalContextType)
> 6   0x109e2b365 JSC::CodeCache::getUnlinkedProgramCodeBlock(JSC::VM&,
> JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserStrictMode,
> WTF::OptionSet<JSC::CodeGenerationMode>, JSC::ParserError&)
> 7   0x10a05d556 JSC::ProgramExecutable::initializeGlobalProperties(JSC::VM&,
> JSC::ExecState*, JSC::JSScope*)
> 8   0x109b0b7ac JSC::Interpreter::executeProgram(JSC::SourceCode const&,
> JSC::ExecState*, JSC::JSObject*)
> 9   0x109e47505 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&,
> JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
> 10  0x108849f17 runWithOptions(GlobalObject*, CommandLine&, bool&)
> 11  0x10881f75a jscmain(int, char**)::$_6::operator()(JSC::VM&,
> GlobalObject*, bool&) const
> 12  0x1087fe804 int runJSC<jscmain(int, char**)::$_6>(CommandLine const&,
> bool, jscmain(int, char**)::$_6 const&)
> 13  0x1087fcf31 jscmain(int, char**)
> 14  0x1087fcd9e main
> 15  0x7fff54f22015 start
> 16  0x8
> 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/3251

I'm not sure what's causing that ASSERT to fail. I'll rollout this patch and investigate.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 199871

[Christopher Reid]

It looks like the failure is because switching to encodeForFileName pushes the bytecode filename slightly over the max file size limit on the bot's environment.

The bytecode file name now ends up as: `5282585-%2FVolumes%2FData%2Fslave%2Fhighsierra-debug-tests-jsc%2Fbuild%2FWebKitBuild%2FDebug%2fjsc-stress-results%2F.tests%2Fstress%2Ftricky-indirectly-inferred-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js.bytecode-cache`
which is 266 characters.

I can re-land this with a call to FileSystem::lastComponentOfPathIgnoringTrailingSlash before encodeForFileName to avoid hitting that issue.

[Christopher Reid]

Created attachment 374335 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 374335 [details]
Patch for landing

Clearing flags on attachment: 374335

Committed r247542: <https://trac.webkit.org/changeset/247542>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.