...
I don't think we need to keep the file locked after we mmap'd it. Since we are using MAP_PRIVATE, we won't see the updates until the next time we mmap it.
I don’t think that’s how mmap works. I think it’s probably unspecified, but if I had to guess, if we mmap it in, then someone else changes content on disk, and then we page fault in that new content on a read, we’ll see the updated content. But there’s a more interesting reason we want to keep it locked: we want to update the API to iteratively update the cache
One man page agrees with me: “MAP_SHARED and MAP_PRIVATE describe the disposition of write references to the memory object. If MAP_SHARED is specified, write references change the underlying object. If MAP_PRIVATE is specified, modifications to the mapped data by the calling process will be visible only to the calling process and will not change the underlying object. It is unspecified whether modifications to the underlying object done after the MAP_PRIVATE mapping is established are visible through the MAP_PRIVATE mapping. Either MAP_SHARED or MAP_PRIVATE can be specified, but not both. The mapping type is retained across fork().” https://pubs.opengroup.org/onlinepubs/7908799/xsh/mmap.html
Yes, you're right, I came across the same thing and was about to comment too. Either way, I don't think we should keep the file locked for updates, since that would require an exclusive lock on the file, so only one process would be able to use the cache at a time. I think we should keep the shared lock during the script execution and only acquire the exclusive lock when we want to write to the cache.
(In reply to Tadeu Zagallo from comment #4) > Yes, you're right, I came across the same thing and was about to comment too. > > Either way, I don't think we should keep the file locked for updates, since > that would require an exclusive lock on the file, so only one process would > be able to use the cache at a time. I think we should keep the shared lock > during the script execution and only acquire the exclusive lock when we want > to write to the cache. I think it makes more sense to make the cache file locked to a single VM at a time. It just simplifies thinking about two VMs concurrently iteratively updating it. That use will not be something we need to support. We could relax this in the future, but I think we should start with the more constrained semantics and only open it up once we're sure we can support such use cases.
Created attachment 363564 [details] patch
Comment on attachment 363564 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=363564&action=review Nice, I agree this makes sense for now. > Source/JavaScriptCore/API/JSScript.mm:190 > + int fd = open([m_cachePath path].UTF8String, O_CREAT | O_RDWR, 0666); Not this diff, but you can combine the locking with open by doing open([m_cachePath path].UTF8String, O_CREAT | O_RDWR | O_EXLOCK | O_NONBLOCK, 0666) I did it somewhere else, but forgot it here.
Comment on attachment 363564 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=363564&action=review >> Source/JavaScriptCore/API/JSScript.mm:190 >> + int fd = open([m_cachePath path].UTF8String, O_CREAT | O_RDWR, 0666); > > Not this diff, but you can combine the locking with open by doing > > open([m_cachePath path].UTF8String, O_CREAT | O_RDWR | O_EXLOCK | O_NONBLOCK, 0666) > > I did it somewhere else, but forgot it here. Nice. I'll fix.
Comment on attachment 363564 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=363564&action=review r=me. > Source/JavaScriptCore/API/JSScript.mm:54 > - UniquedStringImpl* m_moduleKey; > + int m_cacheFileDescriptor; Can you use C++ initializers here? e.g. { -1 }
Comment on attachment 363564 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=363564&action=review >> Source/JavaScriptCore/API/JSScript.mm:54 >> + int m_cacheFileDescriptor; > > Can you use C++ initializers here? e.g. { -1 } Doesn't seem like we can. This is a compile error.
Created attachment 363821 [details] patch for landing
Comment on attachment 363821 [details] patch for landing Clearing flags on attachment: 363821 Committed r242585: <https://trac.webkit.org/changeset/242585>
All reviewed patches have been landed. Closing bug.
<rdar://problem/48662679>