[Webkit-unassigned] [Bug 132229] Coalesce responses on network process side

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=132229





--- Comment #11 from Alexey Proskuryakov <ap at webkit.org>  2014-04-29 09:59:53 PST ---
(From update of attachment 230377)
View in context: https://bugs.webkit.org/attachment.cgi?id=230377&action=review

> Source/WebKit2/ChangeLog:8
> +        To reduce IPC we should coalesce responses in network process and send them over with single IPC call.

I don't object to this change, however I also don't understand why "reducing IPC" improves performance.

I understand that we see a lot of IPC on profiles, but combining responses into one doesn't get rid of any serialization that we perform - we still need to send requests and responses as many times as before, don't we?

A better understanding of where the benefit comes from may help come up with a safer fix that won't be a long-term liability due to added complexity.

> Source/WebKit2/ChangeLog:39
> +            Add allowResponseCoalescing flag. Main resource and XHR is not allowed to coalesce as it may alter
> +            observable semantics.

One place where coalescing would be a substantial win is actually most XHR use cases. We don't need the partial data for Blob responses, we don't need it for XML responses, and so on. Yet we still need progress notifications for these.

For blobs especially, what we do today is buffer the data on WebProcess side, and then send it back to NetworkProcess to register as a blob - crazy!

> Source/WebKit2/NetworkProcess/AsynchronousNetworkLoaderClient.cpp:71
> +        static const double responseCoalescingTime = 0.5;

How did you come up with this number?

This seems to mean that slow loading pages will now be committed half a second later than before, which I expect to be observable.

> Source/WebKit2/NetworkProcess/AsynchronousNetworkLoaderClient.cpp:88
> +            loader->send(Messages::WebResourceLoader::DidReceiveResponseWithCertificateInfo(m_coalescingResponse, CertificateInfo(m_coalescingResponse), false));

This looks like the best place to send a coalesced response - we already have all the data. What is the reason to not use this technique here?

> Source/WebKit2/NetworkProcess/AsynchronousNetworkLoaderClient.h:61
> +    long long m_coalescingResponseEncodedDataLength;

Is a signed value appropriate here?

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:137
> +    Ref<WebResourceLoader> protect(*this);

I'm somewhat worried that protecting only the loader is insufficient. I don't know to to tell what would be sufficient. Maybe run API tests with GuardMalloc? But the coverage is very sketchy.

Maybe it's OK because the loader protects both Frame and DocumentLoader. But it doesn't protect the Document, which can be closed and replaced.

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:143
> +        didReceiveData(data, encodedDataLength);

Seems like we shouldn't do this if client cancels the load while handling didReceiveResponse. Or does m_coreLoader become null in this case?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.