Summary: | Copy-on-write semantics should be an internal implementation detail of StorageMap | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, beidson, ggaren, kkinnunen, sihui_liu, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=225065 | ||||||||
Attachments: |
|
Description
Chris Dumez
2021-04-27 09:04:41 PDT
Created attachment 427157 [details]
Patch
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? (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. Created attachment 427182 [details]
Patch
Comment on attachment 427182 [details] Patch Clearing flags on attachment: 427182 Committed r276659 (237086@main): <https://commits.webkit.org/237086@main> All reviewed patches have been landed. Closing bug. |