Copy-on-write semantics should be an internal implementation detail of StorageMap

[Chris Dumez]

Copy-on-write semantics should be an internal implementation details of StorageMap, instead of requiring the client to replace its storageMap explicitly.

[Chris Dumez]

Created attachment 427157 [details]
Patch

[Sihui Liu]

Comment on attachment 427157 [details]
Patch

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

> Source/WebKit/ChangeLog:17
> +        Instead of making the StorageMap RefCounted and requiring the client to potentially
> +        replace its StorageMap whenever it calls functions that modify the StorageMap, the
> +        copy-on-write semantics in now an internal implementation detail of StorageMap.
> +
> +        To achieve this, the following changes were made:
> +        - StorageMap is no longer RefCounted. Instead, it has an internal Impl data member 
> +          that is RefCounted.
> +        - The internal Impl data member is the one that gets copied on write.
> +        - Functions that modify the StorageMap no longer need to return a StorageMap.
> +        - Add a clear() function for convenience.

These seem to belong to WebCore/ChangeLog

> Source/WebKitLegacy/ChangeLog:17
> +        Instead of making the StorageMap RefCounted and requiring the client to potentially
> +        replace its StorageMap whenever it calls functions that modify the StorageMap, the
> +        copy-on-write semantics in now an internal implementation detail of StorageMap.
> +
> +        To achieve this, the following changes were made:
> +        - StorageMap is no longer RefCounted. Instead, it has an internal Impl data member 
> +          that is RefCounted.
> +        - The internal Impl data member is the one that gets copied on write.
> +        - Functions that modify the StorageMap no longer need to return a StorageMap.
> +        - Add a clear() function for convenience.

Ditto

> Source/WebCore/storage/StorageMap.h:57
> +    bool isShared() const { return !m_impl->hasOneRef(); }

(Maybe a silly question) Is this possible to return true with current implementation?

[Chris Dumez]

(In reply to Sihui Liu from comment #2)
> Comment on attachment 427157 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427157&action=review
> 
> > Source/WebKit/ChangeLog:17
> > +        Instead of making the StorageMap RefCounted and requiring the client to potentially
> > +        replace its StorageMap whenever it calls functions that modify the StorageMap, the
> > +        copy-on-write semantics in now an internal implementation detail of StorageMap.
> > +
> > +        To achieve this, the following changes were made:
> > +        - StorageMap is no longer RefCounted. Instead, it has an internal Impl data member 
> > +          that is RefCounted.
> > +        - The internal Impl data member is the one that gets copied on write.
> > +        - Functions that modify the StorageMap no longer need to return a StorageMap.
> > +        - Add a clear() function for convenience.
> 
> These seem to belong to WebCore/ChangeLog
> 
> > Source/WebKitLegacy/ChangeLog:17
> > +        Instead of making the StorageMap RefCounted and requiring the client to potentially
> > +        replace its StorageMap whenever it calls functions that modify the StorageMap, the
> > +        copy-on-write semantics in now an internal implementation detail of StorageMap.
> > +
> > +        To achieve this, the following changes were made:
> > +        - StorageMap is no longer RefCounted. Instead, it has an internal Impl data member 
> > +          that is RefCounted.
> > +        - The internal Impl data member is the one that gets copied on write.
> > +        - Functions that modify the StorageMap no longer need to return a StorageMap.
> > +        - Add a clear() function for convenience.
> 
> Ditto
> 
> > Source/WebCore/storage/StorageMap.h:57
> > +    bool isShared() const { return !m_impl->hasOneRef(); }
> 
> (Maybe a silly question) Is this possible to return true with current
> implementation?

Sure. See SessionStorageNamespace::cloneTo() and StorageArea::clone(). There are cases where we clone the session data (window.open() & link with target=_blank iirc). In such cases, we initially share the same HashMap to save memory. If one of the 2 pages start modifying the HashMap though, we copy-on-write.

[Chris Dumez]

Created attachment 427182 [details]
Patch

[Chris Dumez]

Comment on attachment 427182 [details]
Patch

Clearing flags on attachment: 427182

Committed r276659 (237086@main): <https://commits.webkit.org/237086@main>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/77228667>