Bug 215012

Summary: Optimize WebsiteDataStoreConfiguration::copy
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Severity: Normal CC: beidson, ggaren, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch none

Description Alex Christensen 2020-07-30 22:40:58 PDT
Optimize WebsiteDataStoreConfiguration::copy
Comment 1 Alex Christensen 2020-07-30 22:41:21 PDT
*** Bug 215011 has been marked as a duplicate of this bug. ***
Comment 2 Alex Christensen 2020-07-30 22:43:38 PDT
Created attachment 405670 [details]
Comment 3 Alex Christensen 2020-07-30 22:43:41 PDT
Comment 4 EWS 2020-07-30 23:16:38 PDT
Committed r265134: <https://trac.webkit.org/changeset/265134>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405670 [details].
Comment 5 Brady Eidson 2020-07-31 00:17:28 PDT
This actually wasn't quite enough! Need to apply the flag to the WebsiteDataStore constructor as well:

WebsiteDataStore::WebsiteDataStore(Ref<WebsiteDataStoreConfiguration>&& configuration, PAL::SessionID sessionID)
    : m_sessionID(sessionID)
    , m_resolvedConfiguration(WTFMove(configuration))
    , m_configuration((m_resolvedConfiguration)->copy()) <-------Here

I'll followup in the morning
Comment 6 Alex Christensen 2020-07-31 08:24:37 PDT
That calls WebsiteDataStoreConfiguration::copy() which benefits from this optimization.
Comment 7 Geoffrey Garen 2020-07-31 09:02:33 PDT
Comment on attachment 405670 [details]

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStoreConfiguration.h:36
> +enum class WillCopyPathsFromExistingConfiguration : bool { No, Yes };

What's important here is not necessarily that the paths came from some other configuration, but rather that the paths are already fully resolved -- right? If so, perhaps a better enum would be PathsAreFullyResolved { No, Yes }. Global knowledge of the code and logical deduction can tell us that a path from an existing configuration is a fully resolved path, but it is best if reading code doesn't require global knowledge or logical deduction.
Comment 8 Brady Eidson 2020-07-31 09:08:08 PDT
(In reply to Alex Christensen from comment #6)
> That calls WebsiteDataStoreConfiguration::copy() which benefits from this
> optimization.

Yup I totally misread it late at night, we're good.
Comment 9 Alex Christensen 2020-07-31 10:27:44 PDT
What's really important here is that we don't do disk read and write operations during WebsiteDataStoreConfiguration::copy.  This was one solution, but I think a better one would be do std::call_once in each of the WebsiteDataStore::default*Directory in WebsiteDataStoreCocoa.mm and remove this strange looking code from WebsiteDataStoreConfiguration.  That's not urgent, though, because this bug is fixed and we don't touch WebsiteDataStoreConfiguration much.