Bug 235897 – Delay writing origin file in NetworkStorageManager

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

rdar://87163253

[Sihui Liu]

Created attachment 450427 [details]
Patch

[Darin Adler]

Comment on attachment 450427 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        The first WebStorage message sent from web process to network process is sync, and writing origin file when 
> +        creating OriginStorageManager will delay the reply. Since we can get the origin information from in-memory map 
> +        when OriginStorageManager is present, we may delay the write to until when OriginStorageManager is destroyed.
> +        This fixes the PLT regression from r286936.

Can we make a regression test for this?

Does it matter if this data gets lost because of a crash?

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:283
> +    if (m_writeOriginFileFunction)
> +        m_writeOriginFileFunction();

It’s often a poor pattern to do side effects like writing to a file in a destructor or constructor. In this case it may be a helpful pattern, but I am a bit concerned that call sites that are removing an element from a map don’t look like they are doing file I/O.

[Sihui Liu]

Comment on attachment 450427 [details]
Patch

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

>> Source/WebKit/ChangeLog:12
>> +        This fixes the PLT regression from r286936.
> 
> Can we make a regression test for this?
> 
> Does it matter if this data gets lost because of a crash?

Tests for the PLT regression? I don't know we have unit test for perf... I can add a comment about why we want to delay the write.
If the network process crashes, we would relaunch a new one. The web process will connect to the new network process and tell it about the origin, so it's probably fine.

>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:283
>> +        m_writeOriginFileFunction();
> 
> It’s often a poor pattern to do side effects like writing to a file in a destructor or constructor. In this case it may be a helpful pattern, but I am a bit concerned that call sites that are removing an element from a map don’t look like they are doing file I/O.

I can add OriginStorageManager::prepareForDestruction, invoking and clearing m_writeOriginFileFunction there, and ensure m_writeOriginFileFunction is null in ~OriginStorageManager(). And let the call sites call prepareForDestruction() before removing OriginStorageManager from map.

[Darin Adler]

Comment on attachment 450427 [details]
Patch

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

>>> Source/WebKit/ChangeLog:12
>>> +        This fixes the PLT regression from r286936.
>> 
>> Can we make a regression test for this?
>> 
>> Does it matter if this data gets lost because of a crash?
> 
> Tests for the PLT regression? I don't know we have unit test for perf... I can add a comment about why we want to delay the write.
> If the network process crashes, we would relaunch a new one. The web process will connect to the new network process and tell it about the origin, so it's probably fine.

We should look for ways to test that we aren’t doing I/O when we don’t intend to; I suspect we could probably invent something that would make a mistake much easier to notice than bisecting a performance regression. Not saying it’s easy.

>>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:283
>>> +        m_writeOriginFileFunction();
>> 
>> It’s often a poor pattern to do side effects like writing to a file in a destructor or constructor. In this case it may be a helpful pattern, but I am a bit concerned that call sites that are removing an element from a map don’t look like they are doing file I/O.
> 
> I can add OriginStorageManager::prepareForDestruction, invoking and clearing m_writeOriginFileFunction there, and ensure m_writeOriginFileFunction is null in ~OriginStorageManager(). And let the call sites call prepareForDestruction() before removing OriginStorageManager from map.

A pattern like that might be better, but I’m not sure it’s necessary.

[Sihui Liu]

Comment on attachment 450427 [details]
Patch

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

>>>> Source/WebKit/ChangeLog:12
>>>> +        This fixes the PLT regression from r286936.
>>> 
>>> Can we make a regression test for this?
>>> 
>>> Does it matter if this data gets lost because of a crash?
>> 
>> Tests for the PLT regression? I don't know we have unit test for perf... I can add a comment about why we want to delay the write.
>> If the network process crashes, we would relaunch a new one. The web process will connect to the new network process and tell it about the origin, so it's probably fine.
> 
> We should look for ways to test that we aren’t doing I/O when we don’t intend to; I suspect we could probably invent something that would make a mistake much easier to notice than bisecting a performance regression. Not saying it’s easy.

hmm, it occurs to me that maybe we can set up auto perf tests for LocalStorage API specifically; we have similar tests for IndexedDB. I will check how that runs.

>>>> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:283
>>>> +        m_writeOriginFileFunction();
>>> 
>>> It’s often a poor pattern to do side effects like writing to a file in a destructor or constructor. In this case it may be a helpful pattern, but I am a bit concerned that call sites that are removing an element from a map don’t look like they are doing file I/O.
>> 
>> I can add OriginStorageManager::prepareForDestruction, invoking and clearing m_writeOriginFileFunction there, and ensure m_writeOriginFileFunction is null in ~OriginStorageManager(). And let the call sites call prepareForDestruction() before removing OriginStorageManager from map.
> 
> A pattern like that might be better, but I’m not sure it’s necessary.

Yes it's not necessary since OriginStorageManager is only used in NetworkStorageManager; and a comment in NetworkStorageManager should be enough to indicate I/O during destruction. Will land the patch as it is.

[Sihui Liu]

Created attachment 450576 [details]
Patch for landing

[EWS]

Committed r288924 (246658@main): <https://commits.webkit.org/246658@main>

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

[Geoffrey Garen]

Comment on attachment 450576 [details]
Patch for landing

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

> Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:284
> +OriginStorageManager::~OriginStorageManager()
> +{
> +    if (m_writeOriginFileFunction)
> +        m_writeOriginFileFunction();
> +}

Is the reason that this change avoids a sync delay because OriginStorageManager is usually destroyed by a call to m_queue->dispatch()? If so, could we just m_queue->dispatch() a call to writeOriginToFileIfNecessary when we construct the OriginStorageManager instead? (To resolve the concern that file I/O in a destructor can be surprising.)

[Sihui Liu]

(In reply to Geoffrey Garen from comment #8)
> Comment on attachment 450576 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450576&action=review
> 
> > Source/WebKit/NetworkProcess/storage/OriginStorageManager.cpp:284
> > +OriginStorageManager::~OriginStorageManager()
> > +{
> > +    if (m_writeOriginFileFunction)
> > +        m_writeOriginFileFunction();
> > +}
> 
> Is the reason that this change avoids a sync delay because
> OriginStorageManager is usually destroyed by a call to m_queue->dispatch()?
> If so, could we just m_queue->dispatch() a call to
> writeOriginToFileIfNecessary when we construct the OriginStorageManager
> instead? (To resolve the concern that file I/O in a destructor can be
> surprising.)

ah scheduling a task to write origin file may also work; I will schedule PLT test to confirm. If it does give us the progression, I will update the patch.

[Sihui Liu]

Reopening to attach new patch.

[Sihui Liu]

Created attachment 450782 [details]
Patch

[Sihui Liu]

Created attachment 450789 [details]
Patch

[Sihui Liu]

Created attachment 450790 [details]
Patch

[Geoffrey Garen]

Comment on attachment 450790 [details]
Patch

r=me

[Sihui Liu]

Created attachment 450817 [details]
Patch for landing

[EWS]

Committed r289081 (246785@main): <https://commits.webkit.org/246785@main>

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