Bug 199759 - Bytecode cache should use FileSystem
Summary: Bytecode cache should use FileSystem
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Christopher Reid
URL:
Keywords: InRadar
Depends on: 199871
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-12 14:28 PDT by Christopher Reid
Modified: 2019-07-17 16:03 PDT (History)
18 users (show)

See Also:


Attachments
patch (32.47 KB, patch)
2019-07-12 14:53 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch (32.46 KB, patch)
2019-07-12 14:57 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Updated Patch (32.45 KB, patch)
2019-07-12 21:58 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Updated Patch (32.28 KB, patch)
2019-07-15 16:37 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch for landing (34.43 KB, patch)
2019-07-16 21:15 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch for landing (34.61 KB, patch)
2019-07-17 15:08 PDT, Christopher Reid
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Reid 2019-07-12 14:28:52 PDT
It currently uses posix file and memory mapping calls.
Comment 1 Christopher Reid 2019-07-12 14:53:57 PDT
Created attachment 374039 [details]
patch
Comment 2 Build Bot 2019-07-12 14:55:29 PDT
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.
Comment 3 Christopher Reid 2019-07-12 14:57:18 PDT
Created attachment 374040 [details]
Patch

style fixes
Comment 4 Tadeu Zagallo 2019-07-12 15:34:55 PDT
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?
Comment 5 Christopher Reid 2019-07-12 15:56:09 PDT
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.
Comment 6 Michael Catanzaro 2019-07-12 19:18:06 PDT
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.
Comment 7 Christopher Reid 2019-07-12 21:54:40 PDT
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.
Comment 8 Christopher Reid 2019-07-12 21:58:45 PDT
Created attachment 374077 [details]
Updated Patch
Comment 9 Stephan Szabo 2019-07-15 16:01:35 PDT
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?
Comment 10 Christopher Reid 2019-07-15 16:33:43 PDT
(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.
Comment 11 Christopher Reid 2019-07-15 16:37:29 PDT
Created attachment 374164 [details]
Updated Patch
Comment 12 Yusuke Suzuki 2019-07-16 16:59:06 PDT
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.
Comment 13 Christopher Reid 2019-07-16 19:29:21 PDT
(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.
Comment 14 Christopher Reid 2019-07-16 21:15:48 PDT
Created attachment 374278 [details]
Patch for landing
Comment 15 WebKit Commit Bot 2019-07-16 22:21:42 PDT
Comment on attachment 374278 [details]
Patch for landing

Clearing flags on attachment: 374278

Committed r247505: <https://trac.webkit.org/changeset/247505>
Comment 16 WebKit Commit Bot 2019-07-16 22:21:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2019-07-16 22:22:19 PDT
<rdar://problem/53188300>
Comment 18 Ryan Haddad 2019-07-17 10:11:32 PDT
(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
Comment 19 Christopher Reid 2019-07-17 10:54:27 PDT
(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.
Comment 20 WebKit Commit Bot 2019-07-17 11:00:05 PDT
Re-opened since this is blocked by bug 199871
Comment 21 Christopher Reid 2019-07-17 14:28:32 PDT
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.
Comment 22 Christopher Reid 2019-07-17 15:08:36 PDT
Created attachment 374335 [details]
Patch for landing
Comment 23 WebKit Commit Bot 2019-07-17 16:03:31 PDT
Comment on attachment 374335 [details]
Patch for landing

Clearing flags on attachment: 374335

Committed r247542: <https://trac.webkit.org/changeset/247542>
Comment 24 WebKit Commit Bot 2019-07-17 16:03:33 PDT
All reviewed patches have been landed.  Closing bug.