Bug 224015 – Have the ServiceWorker process hold on to a file mapped version of the service worker scripts to save dirty memory

• Component: Service Workers
• Status: RESOLVED FIXED

[Chris Dumez]

Once saved to disk, then mmap'd data of service worker scripts back to the ServiceWorker process, in order to save dirty memory. This is similar to what we do in the network cache implementation.

[Chris Dumez]

Created attachment 424818 [details]
Patch

[Chris Dumez]

Created attachment 424855 [details]
Patch

[Chris Dumez]

Created attachment 424857 [details]
Patch

[Chris Dumez]

<rdar://75637679>

[Darin Adler]

Comment on attachment 424857 [details]
Patch

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

> Source/WebCore/platform/SharedBuffer.cpp:224
> +    unsigned count = 0;
> +    for (auto it = begin(); it != end(); ++it)
> +        ++count;
> +    return count;

This can be written like this:

    return std::count(begin(), end());

But also, not sure we want this function. I think hasOneSegment would be a better function to add. It would be something like this:

    auto it = begin();
    return it != end() && ++it == end();

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:117
> +        if (buffer.numberOfSegments() != 1)

This is an inefficient way to check If there is exactly one segment if we have a buffer with a lot segments in it!

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 424857 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424857&action=review
> 
> > Source/WebCore/platform/SharedBuffer.cpp:224
> > +    unsigned count = 0;
> > +    for (auto it = begin(); it != end(); ++it)
> > +        ++count;
> > +    return count;
> 
> This can be written like this:
> 
>     return std::count(begin(), end());
> 
> But also, not sure we want this function. I think hasOneSegment would be a
> better function to add. It would be something like this:
> 
>     auto it = begin();
>     return it != end() && ++it == end();
> 
> > Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerToContextConnection.cpp:117
> > +        if (buffer.numberOfSegments() != 1)
> 
> This is an inefficient way to check If there is exactly one segment if we
> have a buffer with a lot segments in it!

Ok. Will address. Please don't review this iteration further. This patch is very outdated and I will upload a fresh version shortly.

[Darin Adler]

(In reply to Chris Dumez from comment #6)
> Please don't review this iteration further. This patch is
> very outdated and I will upload a fresh version shortly.

No worries, I won’t.

[Chris Dumez]

Created attachment 424908 [details]
Patch

[Geoffrey Garen]

Comment on attachment 424908 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:13
> +        scripts to the ServiceWorker processes as ShareableResource handles so that the dirty memory usage

so that => in order to reduce

> Source/WebCore/ChangeLog:16
> +        No new tests, no Web-facing behavior change, just a decrease dirty memory use in the ServiceWorker

decrease => decrease in

[Geoffrey Garen]

Comment on attachment 424908 [details]
Patch

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

> Source/WebCore/ChangeLog:19
> +        memory usage goes from ~440MB to ~230MB in both the cold and warm cases ("Cold" meaning that the

I wonder if, in the cold case, executing the service worker script causes JavaScriptCore to retain a large dirty memory string. Maybe that explains the remaining 230MB?

[Chris Dumez]

(In reply to Geoffrey Garen from comment #10)
> Comment on attachment 424908 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424908&action=review
> 
> > Source/WebCore/ChangeLog:19
> > +        memory usage goes from ~440MB to ~230MB in both the cold and warm cases ("Cold" meaning that the
> 
> I wonder if, in the cold case, executing the service worker script causes
> JavaScriptCore to retain a large dirty memory string. Maybe that explains
> the remaining 230MB?

230MB remain in the WebProcess in both the cold and warm cases. This seems to indicate both 100MB scripts are still kept in memory in the WebProcess in one place. Looking at the code, I *think* it must in JavaScriptCore indeed. I can try and find out why in a follow-up.

[Chris Dumez]

Created attachment 424927 [details]
Patch

[Darin Adler]

Maybe a garbage-collectable string that just hasn’t been collected yet?

[Chris Dumez]

(In reply to Darin Adler from comment #13)
> Maybe a garbage-collectable string that just hasn’t been collected yet?

Maybe but I did send a memory pressure warning before collecting memory measurements. The memory pressure warning *should* have triggered a sync GC I think.

[Chris Dumez]

(In reply to Chris Dumez from comment #14)
> (In reply to Darin Adler from comment #13)
> > Maybe a garbage-collectable string that just hasn’t been collected yet?
> 
> Maybe but I did send a memory pressure warning before collecting memory
> measurements. The memory pressure warning *should* have triggered a sync GC
> I think.

Note that the scripts I used are very simple:
large.js:
```
importScripts('large-sub.js');
a = 1;
/* a 100MB comment */
```

large-sub.js:
```
b = 2;
/* a 100 MB comment */
```

[EWS]

Committed r275375: <https://commits.webkit.org/r275375>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424927 [details].

[Chris Dumez]

(In reply to Darin Adler from comment #13)
> Maybe a garbage-collectable string that just hasn’t been collected yet?

Turns out we were not clearing the JS code cache or doing GC in worker threads on memory pressure. I followed up in Bug 224110.