JSScript API should only take ascii files.
Created attachment 359101 [details] Patch
Comment on attachment 359101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359101&action=review > Source/JavaScriptCore/ChangeLog:3 > + JSScript API should only take ascii files. Why? ASCII is not cool.
Comment on attachment 359101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359101&action=review >> Source/JavaScriptCore/ChangeLog:3 >> + JSScript API should only take ascii files. > > Why? ASCII is not cool. It's that or UTF16... WTF doesn't support UTF-8 strings. I guess it could be Latin1?
> WTF doesn't support UTF-8 strings. Which part of WTF doesn't? The code was already using String::fromUTF8WithLatin1Fallback... I would say that we don't want latin-1 fallback in an API, but other than that, not sure what the issue is.
(In reply to Alexey Proskuryakov from comment #4) > > WTF doesn't support UTF-8 strings. > > Which part of WTF doesn't? The code was already using > String::fromUTF8WithLatin1Fallback... I would say that we don't want latin-1 > fallback in an API, but other than that, not sure what the issue is. Sorry, I mean without modifying the source data. This API is intended to mmap a file so the source doesn't count against dirty memory. (which it doesn't do yet). StringImpls are either latin1 or UTF-16, AFAIK.
I see. It seems not great to shape an API based on current low level implementation details. I would suggest making the API universal, and if it has an optimized path that certain clients can opt into, good for those clients. As you already iterate over all the characters in the source, you could keep it as scriptFromUTF8File, and use mmapped buffer if the check passes. Alternatively, why not make mmapping client's responsibility? There is enough to worry about that JavaScriptCore is not prepared for in general, such as concurrent writing to the file, or unmounting the file system. JSC can't make a file based API work predictably in those cases, so it seems better leaving to the client to decide what to do there, or to guarantee that they don't care.
Why can't we just have a constructor called JSASCIIScript or something similar so the name dictates this property?
Comment on attachment 359101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359101&action=review > Source/JavaScriptCore/API/JSScript.mm:98 > + for (char c : buffer) { > + if (!isASCII(c)) > + return nil; > + } Do we really want to pull the entire file into memory? Why can't we just debug assert this? If we move to properly naming this something like JSASCIIScript...
Comment on attachment 359101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359101&action=review >> Source/JavaScriptCore/API/JSScript.mm:98 >> + } > > Do we really want to pull the entire file into memory? Why can't we just debug assert this? If we move to properly naming this something like JSASCIIScript... Are you saying that we should rename the class or this constructor? I'm not sure that we only ever want to support ASCII scripts. We could also have a scriptFromUTF16 SPI constructor in the future. As far as pulling the whole file into memory. There is a FIXME to mmap the contents of the file so it shouldn't count against us. Also, we are going to call a code signing function that is going to hash the entire file for signing anyway.
Comment on attachment 359101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359101&action=review >>> Source/JavaScriptCore/API/JSScript.mm:98 >>> + } >> >> Do we really want to pull the entire file into memory? Why can't we just debug assert this? If we move to properly naming this something like JSASCIIScript... > > Are you saying that we should rename the class or this constructor? I'm not sure that we only ever want to support ASCII scripts. We could also have a scriptFromUTF16 SPI constructor in the future. > > As far as pulling the whole file into memory. There is a FIXME to mmap the contents of the file so it shouldn't count against us. Also, we are going to call a code signing function that is going to hash the entire file for signing anyway. Good point regarding pulling in all memory. I think having the name just in this constructor function is fine.
Comment on attachment 359101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359101&action=review > Source/JavaScriptCore/API/JSScript.h:58 > ++ (nullable instancetype)scriptFromASCIIFile:(NSURL *)filePath inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath; Still looks really bad to me as an API, Saam's r+ notwithstanding.
Created attachment 359574 [details] Patch for landing
Comment on attachment 359574 [details] Patch for landing Clearing flags on attachment: 359574 Committed r240194: <https://trac.webkit.org/changeset/240194>
All reviewed patches have been landed. Closing bug.
<rdar://problem/47404335>
(In reply to Alexey Proskuryakov from comment #11) > Comment on attachment 359101 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=359101&action=review > > > Source/JavaScriptCore/API/JSScript.h:58 > > ++ (nullable instancetype)scriptFromASCIIFile:(NSURL *)filePath inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath; > > Still looks really bad to me as an API, Saam's r+ notwithstanding. Can you propose an alternative SPI?
(In reply to Saam Barati from comment #16) > (In reply to Alexey Proskuryakov from comment #11) > > Comment on attachment 359101 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=359101&action=review > > > > > Source/JavaScriptCore/API/JSScript.h:58 > > > ++ (nullable instancetype)scriptFromASCIIFile:(NSURL *)filePath inVirtualMachine:(JSVirtualMachine *)vm withCodeSigning:(nullable NSURL *)codeSigningPath andBytecodeCache:(nullable NSURL *)cachePath; > > > > Still looks really bad to me as an API, Saam's r+ notwithstanding. > > Can you propose an alternative SPI? This SPI was decided upon because we were asked to manage the bytecode cache. Since, we need to read and write files for that part of the SPI it seems reasonable that the whole SPI would handle files, IMO.
> Can you propose an alternative SPI? I did already. First of all, scriptFromUTF8File was perfectly fine as API, as you could choose to use a memory mapped buffer after checking the content. Secondly, I recommend against dealing with files, especially for something long lived like mmap. The client that you have in mind today may not care, but future clients may have expectations with regards to wasting open file handles, or to how the process responds to file system unmounting or flakiness. The documentation doesn't even call it out that there will be side effects beyond creating the script. > This SPI was decided upon because we were asked to manage the bytecode cache. Since, we need to read and write files for that part of the SPI it seems reasonable that the whole SPI would handle files, IMO. For the cache, you can have reasonable expectations for file system staying in place, as well as whether files will be deleted underneath you.
(In reply to Alexey Proskuryakov from comment #18) > > Can you propose an alternative SPI? > > I did already. > > First of all, scriptFromUTF8File was perfectly fine as API, as you could > choose to use a memory mapped buffer after checking the content. Since this is SPI, I think it makes sense for the SPI to fail instead of silently opt you into using more memory than you expected. When using a lower level SPI like this, we want clients to know when they mess up and opt themselves back into using more memory than they wanted to. The whole point of using this SPI is to use less memory. It feels weird doing something that might cost the client memory when they were not expecting it. > > Secondly, I recommend against dealing with files, especially for something > long lived like mmap. The client that you have in mind today may not care, > but future clients may have expectations with regards to wasting open file > handles, or to how the process responds to file system unmounting or > flakiness. The documentation doesn't even call it out that there will be > side effects beyond creating the script. > > > This SPI was decided upon because we were asked to manage the bytecode cache. Since, we need to read and write files for that part of the SPI it seems reasonable that the whole SPI would handle files, IMO. > > For the cache, you can have reasonable expectations for file system staying > in place, as well as whether files will be deleted underneath you.
(In reply to Alexey Proskuryakov from comment #18) > > Can you propose an alternative SPI? > > I did already. I see. Sorry I missed that comment... > > First of all, scriptFromUTF8File was perfectly fine as API, as you could > choose to use a memory mapped buffer after checking the content. > > Secondly, I recommend against dealing with files, especially for something > long lived like mmap. The client that you have in mind today may not care, > but future clients may have expectations with regards to wasting open file > handles, or to how the process responds to file system unmounting or > flakiness. The documentation doesn't even call it out that there will be > side effects beyond creating the script. > > > This SPI was decided upon because we were asked to manage the bytecode cache. Since, we need to read and write files for that part of the SPI it seems reasonable that the whole SPI would handle files, IMO. > > For the cache, you can have reasonable expectations for file system staying > in place, as well as whether files will be deleted underneath you.
(In reply to Alexey Proskuryakov from comment #6) > Alternatively, why not make mmapping client's responsibility? There is > enough to worry about that JavaScriptCore is not prepared for in general, > such as concurrent writing to the file, or unmounting the file system. JSC > can't make a file based API work predictably in those cases, so it seems > better leaving to the client to decide what to do there, or to guarantee > that they don't care. I mostly agree with this. I think it'd be nicer if it were the client's responsibility. The only thing that gives me some pause is if we recommend this to N different clients, and they implement it N different ways, perhaps we should implement it ourselves. But maybe that's a bridge we should cross when we get there.
> The whole point of using this SPI is to use less memory. I see. Not sure how huge the files in question are. In any case, it seems like setting up a memory use test would be a better long term way to make sure that memory use doesn't grow. There are certainly many things we can do that increase memory use, and having an SPI guarantee that there is no copy at the very edge doesn't do very much to defend the memory use goal.