Bug 215012 – Optimize WebsiteDataStoreConfiguration::copy

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Optimize WebsiteDataStoreConfiguration::copy

[Alex Christensen]

*** Bug 215011 has been marked as a duplicate of this bug. ***

[Alex Christensen]

Created attachment 405670 [details]
Patch

[Alex Christensen]

<rdar://problem/64263406>

[EWS]

Committed r265134: <https://trac.webkit.org/changeset/265134>

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

[Brady Eidson]

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

[Alex Christensen]

That calls WebsiteDataStoreConfiguration::copy() which benefits from this optimization.

[Geoffrey Garen]

Comment on attachment 405670 [details]
Patch

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.

[Brady Eidson]

(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.

[Alex Christensen]

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.