Summary: | Image data should be coalesced if it comes in small chunks before updating the ImageSource | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||
Component: | Images | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, darin, dbates, dino, esprehn+autocc, gyuyoung.kim, japhet, simon.fraser, thorton, webkit-bug-importer, youennf | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=178020 | ||||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2017-08-23 10:40:23 PDT
Created attachment 318886 [details]
Patch
Comment on attachment 318886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318886&action=review > Source/WebCore/loader/cache/CachedImage.cpp:450 > + ASSERT(m_coalesceDataCount < coalesceDataSeconds.get().size()); This assert seems super unnecessary especially now that it's a Vector instead of a C array. > Source/WebCore/loader/cache/CachedImage.cpp:455 > + m_coalesceDataCount = std::min<unsigned>(m_coalesceDataCount + 1, coalesceDataSeconds.get().size() - 1); Isn't this just a silly way of writing: if (m_coalesceDataCount < coalesceDataSeconds.get().size() - 1) m_coalesceDataCount++; Also, is that the best name? Since we cap it based on the number of items in the durations array, it's more of an index into that array than a count of times we coalesced. > Source/WebCore/loader/cache/CachedImage.cpp:468 > + if (shouldCoalesceDataBuffer()) > + return; I'm confused. How does this not result in the data getting dropped on the floor? Created attachment 319013 [details]
Patch
Comment on attachment 318886 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=318886&action=review >> Source/WebCore/loader/cache/CachedImage.cpp:450 >> + ASSERT(m_coalesceDataCount < coalesceDataSeconds.get().size()); > > This assert seems super unnecessary especially now that it's a Vector instead of a C array. I am not sure if I agree with that. What is the difference between accessing a Vector and a C array with an out-of-bound index? I wanted to ensure that the index does not change outside the scope of this function such that it is larger than the last index in the Vector. But anyway I removed it. >> Source/WebCore/loader/cache/CachedImage.cpp:455 >> + m_coalesceDataCount = std::min<unsigned>(m_coalesceDataCount + 1, coalesceDataSeconds.get().size() - 1); > > Isn't this just a silly way of writing: > > if (m_coalesceDataCount < coalesceDataSeconds.get().size() - 1) > m_coalesceDataCount++; > > Also, is that the best name? Since we cap it based on the number of items in the durations array, it's more of an index into that array than a count of times we coalesced. The std::min() is replaced by the if-statemnt as suggested. The Vector's name is changed to coalesceDataDurations and the index's name is changed to m_currentCoalesceDataDuration. >> Source/WebCore/loader/cache/CachedImage.cpp:468 >> + return; > > I'm confused. How does this not result in the data getting dropped on the floor? It does not because the CachedRawResource passes its SharedBuffer to the CachedImage which just makes makeRef() of it. The CachedRawResource keeps accumulating all the incoming data in the same SharedBuffer. It is guaranteed that the SharedBuffer will be set in the ImageSource at least once in CachedImage::finishLoading(). My purpose of this change was to ensure the data is consistent between the CachedImage and the ImageSource. I thought as long as the ImageSource is not updated with the new data, it is better not to update the CachedImage with the new data. But after realizing that the SharedBuffer of the CachedRawResource is already referenced by the CachedImage, I think it is better to keep the old behavior which can be done by moving the check if (shouldCoalesceDataBuffer()) to addIncrementalDataBuffer(). Comment on attachment 319013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319013&action=review > Source/WebCore/loader/cache/CachedImage.cpp:449 > + static const auto coalesceDataDurations = makeNeverDestroyed(Vector<double> { 0, 1, 3, 6, 15 }); Vector<Seconds>. makeNeverDestroyed is overkill, but I guess you might need it if static Seconds[] doesn't work. > Source/WebCore/loader/cache/CachedImage.cpp:456 > + m_coalesceDataTime = time; A function called shouldCoalesceDataBuffer() should not change state, I think. m_coalesceDataTime should be changed when you actually do the coalescing. > Source/WebCore/loader/cache/CachedImage.h:163 > + size_t m_currentCoalesceDataDuration { 0 }; This isn't a duration, right? So the name is misleading. It's an index into some array. Comment on attachment 319013 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319013&action=review >> Source/WebCore/loader/cache/CachedImage.cpp:449 >> + static const auto coalesceDataDurations = makeNeverDestroyed(Vector<double> { 0, 1, 3, 6, 15 }); > > Vector<Seconds>. > > makeNeverDestroyed is overkill, but I guess you might need it if static Seconds[] doesn't work. It’s not overkill because this is a Vector and it won’t compile otherwise. If this was an array, and it should be an array instead of a vector, then we would not need makeNeverDestroyed. static const double coalesceDataDurations[] = { 0, 1, 3, 6, 15 }; Created attachment 319942 [details]
Patch
Comment on attachment 319942 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319942&action=review > Source/WebCore/loader/cache/CachedImage.cpp:450 > + static const double dataBufferAddingIntervals[] = { 0, 1, 3, 6, 15 }; I think this one should be more clear that it's about backoff. Maybe incrementalLoadingBackoffIntervals? Maybe propagate that naming through all of the function/member/variable names? Right now it's kind of mysterious. e.g. bool CachedImage::shouldDeferIncrementalLoad() const { static const double incrementalLoadingBackoffIntervals[] = { 0, 1, 3, 6, 15 }; unsigned interval = std::min<unsigned>(m_incrementalLoadCount, 4); // The first time through, the chunk time will be 0 and the image will get an update. return (MonotonicTime::now() - m_lastIncrementalLoadingUpdateTime).seconds() < incrementalLoadingBackoffIntervals[interval]; } void CachedImage::didIncrementalLoad() { m_lastIncrementalLoadingUpdateTime = MonotonicTime::now(); ASSERT(m_incrementalLoadCount < std::numeric_limits<unsigned>::max()); ++m_incrementalLoadCount; } I'm not totally sold on the names tho. Maybe someone else has an idea. Created attachment 322264 [details]
Patch
Would it be useful to have that kind of behavior for other types of resources? Or other related behavior like processing only when receiving the last byte? Instead of doing it in the WebProcess, could NetworkProcess be told to adapt its pace of sending data chunks on a per load basis? (In reply to youenn fablet from comment #11) > Would it be useful to have that kind of behavior for other types of > resources? There is no problem in receiving the resource's bytes in small chunks. The problem with the image is updating the decoder with small chunks of data. This is costly because the ImageDecoder has to re-decode the new data. Also repainting and drawing the RenderImage every time an image data is received is expensive. Or other related behavior like processing only when receiving the > last byte? No, we can't do that. We would like to show partial images instead of showing nothing till the whole image data is received. > Instead of doing it in the WebProcess, could NetworkProcess be told to adapt > its pace of sending data chunks on a per load basis? I don't know if this solution is feasible or not. But I think dealing with this issue has to be done based on the resource types. Each resource can decide whether it needs to process any incoming data immediately or it can coalesce the number of processing the incoming data. So this decision has to be in WebProcess. (In reply to Said Abou-Hallawa from comment #12) > (In reply to youenn fablet from comment #11) > > Would it be useful to have that kind of behavior for other types of > > resources? > > There is no problem in receiving the resource's bytes in small chunks. The > problem with the image is updating the decoder with small chunks of data. > This is costly because the ImageDecoder has to re-decode the new data. Also > repainting and drawing the RenderImage every time an image data is received > is expensive. The current patch seems to fix the issue for CachedImage. If a similar optimization can be envisioned for other types of resources, it may be better to do it at a lower layer, the CachedResource layer being responsible to decide which sending they expect. > > Or other related behavior like processing only when receiving the > > last byte? > > No, we can't do that. We would like to show partial images instead of > showing nothing till the whole image data is received. Sure for images, my question is more for other types of resources. > > Instead of doing it in the WebProcess, could NetworkProcess be told to adapt > > its pace of sending data chunks on a per load basis? > > I don't know if this solution is feasible or not. But I think dealing with > this issue has to be done based on the resource types. Each resource can > decide whether it needs to process any incoming data immediately or it can > coalesce the number of processing the incoming data. So this decision has to > be in WebProcess. If this issue is spanning several types of resources, it might be simpler to have CachedResource pass a parameter expressing what it is expecting. For instance, ResourceLoader already does buffering according its dataBufferingPolicy option. Maybe extending this option would be a good way to see whether that approach would work for your use case. If so, I would then suggest to move the buffering to NetworkProcess by passing an additional parameter to NetworkConnectionToWebProcess::ScheduleResourceLoad. That would reduce the number of IPC calls. After looking at the code of NetworkConnectionToWebProcess, I realized that the network process implements a linear back-off buffering for the image resource. WebLoaderStrategy::scheduleLoad() sets the member maximumBufferingTime of the NetworkResourceLoadParameters to 500_ms if the resource type is CachedResource::ImageResource. NetworkConnectionToWebProcess creates a timer with 500_ms and when it fires it sends the received data to the WebProcess. This patch implements an exponential back-off buffering. The timer interval keeps increasing till it reaches the last interval and then it uses this interval till loading the image finishes. I agree having two buffering mechanisms the image resource is bad. I could change NetworkResourceLoadParameters::maximumBufferingTime to be a Vector<Seconds>. And I could change the NetworkResourceLoader to create its timers with the intervals from the Vector<Seconds> one after the other till the last one. But we will have two problems: 1. This solution does not work for WK1. A similar buffering mechanism is needed for WK1 at least for iOS since the exponential buffering was implemented for iOS. We may have two solutions for this: a) WebCoreResourceHandleAsDelegate.didReceiveData() needs to implements the same buffering that NetworkResourceLoader implements. b) Keep the same buffering mechanism in ImageSource::dataChanged() but enable it for for WK1. And this is really ugly because we will have the buffering for the image in two different levels for WK1 and WK2. 2. This solution is mainly needed for ImageDocument for WK2. The bug is not noticeable when the image is included in an <img> element. For example open these images in Safari and notice how bad the performance is: http://www.914world.com/bbs2/uploads_offsite/s6.postimg.org-1623-1505909118.1.gif https://www.nasa.gov/sites/default/files/thumbnails/image/pia21778.png Now open the same images as sub-resources, i.e. create HTML files with the following <img> element: <img src = "http://www.914world.com/bbs2/uploads_offsite/s6.postimg.org-1623-1505909118.1.gif"> and <img src = "https://www.nasa.gov/sites/default/files/thumbnails/image/pia21778.png"> The reason for the difference is "static Seconds maximumBufferingTimes(CachedResource*)" returns 500_ms for the CachedResource::ImageResource but it returns 0_s for CachedResource::MainResource. When the image is the main resource no buffering is made which is the case of the ImageDocument. But when the image is a sub-resource, updating the web process with the image data happens only every 500_ms. And this buffering is enough at least for non iOS platforms. I do not think we can know for sure that the main resource is an image before starting loading it and parsing it. In fact, we create the ImageDocument and the ImageDocumentParse after receiving the first chunk of data. So the buffering is is not implemented for WK1 and ImageDocument. And both are currently not handled by the NetworkProcess. And I think implementing the single image buffering in the network side for Wk1 and ImageDocument is expensive. Having said that, I think having a single image buffering for all platforms is not in the scope of this bug. It is late for me so I may have other feedback tomorrow. Regarding WK1, I agree that the NetworkProcess idea is not fully portable to WK1. At this point, not optimizing WK1 might be ok if we think this is the right solution for WK2. I would check how this idea would fly with the various way images can be loaded (main document, sub resources) and whether doing it in NetworkProcess will or not solve potential problems for other resources. (In reply to youenn fablet from comment #15) > It is late for me so I may have other feedback tomorrow. > > Regarding WK1, I agree that the NetworkProcess idea is not fully portable to > WK1. > At this point, not optimizing WK1 might be ok if we think this is the right > solution for WK2. I disagree because the image buffering is currently implemented for WK1 on iOS in the function ImageSource::dataChanged() and we should not remove it unless we have a better way of doing it. > > I would check how this idea would fly with the various way images can be > loaded (main document, sub resources) and whether doing it in NetworkProcess > will or not solve potential problems for other resources. I filed https://bugs.webkit.org/show_bug.cgi?id=178020 to address youenn's comments in a separate patch. The attached patch improves the performance on WK1 for all images and on WK2 for the ImageDocument by enabling the existing backing off mechanism on all platforms. And I think we are not making anything worse. So this patch is ready for review. Comment on attachment 322264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322264&action=review > Source/WebCore/loader/cache/CachedImage.cpp:411 > +void CachedImage::doUpdateBuffer(SharedBuffer& data) This name is awkward. Maybe updateBufferInternal? > Source/WebCore/loader/cache/CachedImage.cpp:421 > + if (shouldDeferUpdateImageData()) > + return; > + > // Have the image update its data from its internal buffer. > // It will not do anything now, but will delay decoding until The early return after shouldDeferUpdateImageData() and then the comment which says "It will not do anything now" are confusing. If updateImageData don't do anything, why did we early return? Created attachment 323217 [details]
Patch
Comment on attachment 323217 [details] Patch Clearing flags on attachment: 323217 Committed r223091: <http://trac.webkit.org/changeset/223091> All reviewed patches have been landed. Closing bug. |