Bug 195186 – JSScript should keep the cache file locked for the duration of its existence and should truncate the cache when it is out of date

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Tadeu Zagallo]

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.

[Saam Barati]

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

[Saam Barati]

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

[Tadeu Zagallo]

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.

[Saam Barati]

(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.

[Saam Barati]

Created attachment 363564 [details]
patch

[Tadeu Zagallo]

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.

[Saam Barati]

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.

[Keith Miller]

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 }

[Saam Barati]

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.

[Saam Barati]

Created attachment 363821 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 363821 [details]
patch for landing

Clearing flags on attachment: 363821

Committed r242585: <https://trac.webkit.org/changeset/242585>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48662679>