Bug 143339 - [WK2] Storage areas should get torn down when there are no remaining references to them.
Summary: [WK2] Storage areas should get torn down when there are no remaining referenc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords: InRadar, Performance
Depends on:
Blocks:
 
Reported: 2015-04-02 10:22 PDT by Andreas Kling
Modified: 2020-02-03 16:53 PST (History)
9 users (show)

See Also:


Attachments
Patch for EWS (8.59 KB, patch)
2015-04-02 10:23 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch (8.83 KB, patch)
2015-04-02 10:48 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (8.92 KB, patch)
2015-04-02 11:11 PDT, Andreas Kling
darin: review+
Details | Formatted Diff | Diff
Better patch that's less wrong (10.17 KB, patch)
2015-05-26 18:20 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Patch (13.41 KB, patch)
2015-05-26 21:08 PDT, Andreas Kling
darin: review+
Details | Formatted Diff | Diff
Patch for landing (13.49 KB, patch)
2015-05-27 16:43 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2015-04-02 10:22:07 PDT
We shouldn't keep local storage areas open forever.
Comment 1 Andreas Kling 2015-04-02 10:23:13 PDT
Created attachment 249989 [details]
Patch for EWS
Comment 2 Andreas Kling 2015-04-02 10:48:34 PDT
Created attachment 249993 [details]
Proposed patch
Comment 3 Andreas Kling 2015-04-02 11:11:58 PDT
Created attachment 249997 [details]
Patch
Comment 4 Darin Adler 2015-04-02 12:46:30 PDT
Comment on attachment 249997 [details]
Patch

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

> Source/WebKit2/ChangeLog:15
> +        Practically speaking, this means that the garbage collector now decides when local
> +        storage databases can be closed, instead of us keeping them open for the lifetime
> +        of the web process.

When you put it that way, it does seem a bit lame. Is there no way this can be based on some better defined event other than “when the object is garbage collected”?

Further, can we add some extra weight to the objects tat are holding the databases open so they are more likely to be collected even when there is plenty of RAM still available?

> Source/WebKit2/WebProcess/Storage/StorageNamespaceImpl.cpp:86
> +    return StorageAreaImpl::create(map);

Could this be either map.release() or WTF::move(map) to avoid a little churn?
Comment 5 Geoffrey Garen 2015-04-02 12:58:30 PDT
Comment on attachment 249997 [details]
Patch

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

>> Source/WebKit2/ChangeLog:15
>> +        of the web process.
> 
> When you put it that way, it does seem a bit lame. Is there no way this can be based on some better defined event other than “when the object is garbage collected”?
> 
> Further, can we add some extra weight to the objects tat are holding the databases open so they are more likely to be collected even when there is plenty of RAM still available?

Does the spec allow us to close a storage unconditionally after navigation (even if the storage is still referenced by another webpage)?
Comment 6 Andreas Kling 2015-04-02 13:19:33 PDT
(In reply to comment #5)
> Comment on attachment 249997 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249997&action=review
> 
> >> Source/WebKit2/ChangeLog:15
> >> +        of the web process.
> > 
> > When you put it that way, it does seem a bit lame. Is there no way this can be based on some better defined event other than “when the object is garbage collected”?
> > 
> > Further, can we add some extra weight to the objects tat are holding the databases open so they are more likely to be collected even when there is plenty of RAM still available?
> 
> Does the spec allow us to close a storage unconditionally after navigation
> (even if the storage is still referenced by another webpage)?

The intent here is to make a change that's not observable from the web.

Anders pointed out that this change may break session storage, so I am currently looking into that.
Comment 7 David Kilzer (:ddkilzer) 2015-04-08 10:48:13 PDT
<rdar://problem/20156436>
Comment 8 Andreas Kling 2015-05-26 18:10:28 PDT
(In reply to comment #4)
> Comment on attachment 249997 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249997&action=review
> 
> > Source/WebKit2/ChangeLog:15
> > +        Practically speaking, this means that the garbage collector now decides when local
> > +        storage databases can be closed, instead of us keeping them open for the lifetime
> > +        of the web process.
> 
> When you put it that way, it does seem a bit lame. Is there no way this can
> be based on some better defined event other than “when the object is garbage
> collected”?
> 
> Further, can we add some extra weight to the objects tat are holding the
> databases open so they are more likely to be collected even when there is
> plenty of RAM still available?

That would be nice. StorageArea already has this:

    virtual size_t memoryBytesUsedByCache() = 0;

But everyone just implements it as { return 0; }. :-/

> > Source/WebKit2/WebProcess/Storage/StorageNamespaceImpl.cpp:86
> > +    return StorageAreaImpl::create(map);
> 
> Could this be either map.release() or WTF::move(map) to avoid a little churn?

Definitely.
Comment 9 Andreas Kling 2015-05-26 18:20:45 PDT
Created attachment 253766 [details]
Better patch that's less wrong
Comment 10 Andreas Kling 2015-05-26 21:08:12 PDT
Created attachment 253775 [details]
Patch
Comment 11 Darin Adler 2015-05-27 09:08:11 PDT
Comment on attachment 253775 [details]
Patch

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

I think Anders gave this an informal thumbs up. Based on that plus my reading of this, review+.

> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:754
> +        Ref<StorageArea> area = *it.value;
> +        if (!area->isSessionStorage())
> +            continue;
> +        if (!origin->isSameSchemeHostPort(area->securityOrigin()))
> +            continue;

Could restructure this and make it a bit uglier but remove a tiny bit of reference count churn like this:

    auto& area = it.value;
    if (...)
        ...
    Ref<StorageArea> movedArea = WTF::move(area);
    ...
    m_storageAreasByConnection.add({ &connection, storageMapID }, WTF::move(movedArea));

> Source/WebKit2/UIProcess/Storage/StorageManager.cpp:755
> +        m_storageAreasByConnection.remove(it.key);

This should be remove(it) rather than remove(it.key) to avoid re-hashing the key and doing equality comparisons just to find the hash table entry that "it" is already pointing at.

> Source/WebKit2/WebProcess/Storage/StorageAreaMap.cpp:61
>  StorageAreaMap::StorageAreaMap(StorageNamespaceImpl* storageNamespace, Ref<WebCore::SecurityOrigin>&& securityOrigin)

Someone should come back here and make StorageNamespaceImpl a reference.
Comment 12 Andreas Kling 2015-05-27 16:43:14 PDT
Created attachment 253816 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2015-05-27 17:31:16 PDT
Comment on attachment 253816 [details]
Patch for landing

Clearing flags on attachment: 253816

Committed r184930: <http://trac.webkit.org/changeset/184930>
Comment 14 WebKit Commit Bot 2015-05-27 17:31:21 PDT
All reviewed patches have been landed.  Closing bug.