[chromium] Adaptively throttle texture uploads
Created attachment 162672 [details] Patch
Looking for early feedback on this patch. It will need to be tested to make sure it's tuned to run well on a variety of platforms...
Comment on attachment 162672 [details] Patch Attachment 162672 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13785301
Comment on attachment 162672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162672&action=review > Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.cpp:62 > +void ThrottledTextureUploader::Query::end(unsigned pixelsUploaded) bit odd that this is on end rather than begin... any good reason? > Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.cpp:131 > + return m_averagePixelsPerSecond; Should we do this in mpix so the numbers dont overflow? > Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.cpp:169 > + m_averagePixelsPerSecond = (m_averagePixelsPerSecond * (uploadAverageWindow-1) + I dont quite understand this math. Is this a weighted average of some sort? Why not m_average * decay + pps*(1-decay)? I'm not very good at this stuff so maybe I'm missing something. Also, should we use a proper window or a percentile or something? If a texture upload is unlucky to get descheduled, i expect it'll hugely sway any sort of mean. That then forces a very slow decay... ~shrug~ would a histogram make sense? > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:38 > +static const size_t pixelsPerTexture = 256 * 256; this isn't guaranteed... > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:60 > +double CCTextureUpdateController::averagePixelsPerSecond() why this accessor? > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:84 > + if (m_uploader->pendingUploadGroups() < targetUploadGroups) { Is there a simple version of this we can try fist? E.g. just look at the average upload time and, if its less than 0.05 for a 256, then do 4x the current count. Period. No control scheme more than that. I'm a big believer ni starting with something simple that we then can refine...
Comment on attachment 162672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162672&action=review >> Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.cpp:169 >> + m_averagePixelsPerSecond = (m_averagePixelsPerSecond * (uploadAverageWindow-1) + > > I dont quite understand this math. Is this a weighted average of some sort? Why not m_average * decay + pps*(1-decay)? I'm not very good at this stuff so maybe I'm missing something. > > Also, should we use a proper window or a percentile or something? If a texture upload is unlucky to get descheduled, i expect it'll hugely sway any sort of mean. That then forces a very slow decay... ~shrug~ would a histogram make sense? The occasional unlucky upload time was a problem last time I played with this. I was getting better results when averaging the slope of the linear interpolant between each recorded value. > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:111 > + m_textureUpdatesPerTick = ceil(texturesPerSecond * m_textureUpdateTickPeriod * .25 + epsilon); I don't like the idea of setting m_textureUpdatesPerTick to less than the value returned by maxPartialTextureUpdates(). We need to allow uploading of maxPartialTextureUpdates textures not to break partial uploads and that would make the timing estimates here incorrect. To get the most out of increasing m_textureUpdatesPerTick we should also allow more partial updates on the main thread. > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:146 > + if (fullUploadCount && (count - fullUploadCount) < queue->partialUploadSize()) I guess this is to compensate for the fact that m_textureUpdatesPerTick can now be less than maxPartialTextureUpdates. Can we instead adjust maxPartialTextureUpdates so that queue->partialUploadSize() is greater than count? > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.h:85 > + size_t m_sequentialUpdateCount; It's not obvious to me what this member variable is for. Maybe add a comment?
Comment on attachment 162672 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162672&action=review >> Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.cpp:62 >> +void ThrottledTextureUploader::Query::end(unsigned pixelsUploaded) > > bit odd that this is on end rather than begin... any good reason? ThrottledTextureUploader doesn't have the information available to pass to Query::begin. ThrottledTextureUploader accumulates the pixels for each upload and then passes it into Query::end. We could require the caller of ThrottledTextureUploader::beginUploads to pass in that information, but that would require two passes over the pending uploads. >> Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.cpp:131 >> + return m_averagePixelsPerSecond; > > Should we do this in mpix so the numbers dont overflow? Yeah, I'm a little worried about overflow as well. Will come up with something better. >>> Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.cpp:169 >>> + m_averagePixelsPerSecond = (m_averagePixelsPerSecond * (uploadAverageWindow-1) + >> >> I dont quite understand this math. Is this a weighted average of some sort? Why not m_average * decay + pps*(1-decay)? I'm not very good at this stuff so maybe I'm missing something. >> >> Also, should we use a proper window or a percentile or something? If a texture upload is unlucky to get descheduled, i expect it'll hugely sway any sort of mean. That then forces a very slow decay... ~shrug~ would a histogram make sense? > > The occasional unlucky upload time was a problem last time I played with this. I was getting better results when averaging the slope of the linear interpolant between each recorded value. uploadAverageWindow's naming is a little missleading since it is more of a decay, where decay = (window-1)/window in Nat's equation. Something a little smarter will probably be good. Can I leverage any of the work you guys have already done? >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:38 >> +static const size_t pixelsPerTexture = 256 * 256; > > this isn't guaranteed... Yeah, I wasn't sure how to get that information initially. Can the full upload texture sizes change on the fly? Otherwise, I could probably just record the last value of a full texture upload. >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:60 >> +double CCTextureUpdateController::averagePixelsPerSecond() > > why this accessor? Pretty much for the TODO part. If we don't get to the TODO in this patch, I'll remove it. >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:84 >> + if (m_uploader->pendingUploadGroups() < targetUploadGroups) { > > Is there a simple version of this we can try fist? E.g. just look at the average upload time and, if its less than 0.05 for a 256, then do 4x the current count. Period. No control scheme more than that. I'm a big believer ni starting with something simple that we then can refine... Yeah, we can start off with something more simple. I picked a lot of constants in here somewhat arbitrarily and it'll take a lot more time tuning those constants than it would take to tune something simpler. We do have control over the tick rate and the number of textures to upload though, so maybe we can have both of those depend on the average upload time. >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:111 >> + m_textureUpdatesPerTick = ceil(texturesPerSecond * m_textureUpdateTickPeriod * .25 + epsilon); > > I don't like the idea of setting m_textureUpdatesPerTick to less than the value returned by maxPartialTextureUpdates(). We need to allow uploading of maxPartialTextureUpdates textures not to break partial uploads and that would make the timing estimates here incorrect. > > To get the most out of increasing m_textureUpdatesPerTick we should also allow more partial updates on the main thread. Thanks for pointing that out. I see now that CCLayerTreeHost::requestPartialTextureUpdate() actually uses this information to limit the number of partial updates per frame. >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:146 >> + if (fullUploadCount && (count - fullUploadCount) < queue->partialUploadSize()) > > I guess this is to compensate for the fact that m_textureUpdatesPerTick can now be less than maxPartialTextureUpdates. Can we instead adjust maxPartialTextureUpdates so that queue->partialUploadSize() is greater than count? As a first pass for this patch, I think it'll be easiest to do what you suggest. Will we ever want to have CCLayerTreeHost::requestPartialTextureUpdate() take into account a dynamically changing maxPartialTextureUpdates? >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.h:85 >> + size_t m_sequentialUpdateCount; > > It's not obvious to me what this member variable is for. Maybe add a comment? It's supposed to be the number of texture uploads so far since the updateMoreTextures that initiated the first texture upload. It is used to avoid making any adjustments during our first upload since we won't have any feedback yet, which I'm realizing may not necessarily be true now. Will need to rethink it. Also, I think there are some race conditions with vsync ticks that might throw off the way I update this variable.
Comment on attachment 162672 [details] Patch Attachment 162672 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13771874
(In reply to comment #6) > (From update of attachment 162672 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162672&action=review > > >> Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.cpp:62 > >> +void ThrottledTextureUploader::Query::end(unsigned pixelsUploaded) > > > > bit odd that this is on end rather than begin... any good reason? > > ThrottledTextureUploader doesn't have the information available to pass to Query::begin. ThrottledTextureUploader accumulates the pixels for each upload and then passes it into Query::end. We could require the caller of ThrottledTextureUploader::beginUploads to pass in that information, but that would require two passes over the pending uploads. > > >> Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.cpp:131 > >> + return m_averagePixelsPerSecond; > > > > Should we do this in mpix so the numbers dont overflow? > > Yeah, I'm a little worried about overflow as well. Will come up with something better. > > >>> Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.cpp:169 > >>> + m_averagePixelsPerSecond = (m_averagePixelsPerSecond * (uploadAverageWindow-1) + > >> > >> I dont quite understand this math. Is this a weighted average of some sort? Why not m_average * decay + pps*(1-decay)? I'm not very good at this stuff so maybe I'm missing something. > >> > >> Also, should we use a proper window or a percentile or something? If a texture upload is unlucky to get descheduled, i expect it'll hugely sway any sort of mean. That then forces a very slow decay... ~shrug~ would a histogram make sense? > > > > The occasional unlucky upload time was a problem last time I played with this. I was getting better results when averaging the slope of the linear interpolant between each recorded value. > > uploadAverageWindow's naming is a little missleading since it is more of a decay, where decay = (window-1)/window in Nat's equation. > > Something a little smarter will probably be good. Can I leverage any of the work you guys have already done? what I played with was even more complicated. I'd prefer if we started with something really simple, like using the median over the last 10 full update groups. > > >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:38 > >> +static const size_t pixelsPerTexture = 256 * 256; > > > > this isn't guaranteed... > > Yeah, I wasn't sure how to get that information initially. Can the full upload texture sizes change on the fly? Otherwise, I could probably just record the last value of a full texture upload. > > >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:60 > >> +double CCTextureUpdateController::averagePixelsPerSecond() > > > > why this accessor? > > Pretty much for the TODO part. If we don't get to the TODO in this patch, I'll remove it. > > >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:84 > >> + if (m_uploader->pendingUploadGroups() < targetUploadGroups) { > > > > Is there a simple version of this we can try fist? E.g. just look at the average upload time and, if its less than 0.05 for a 256, then do 4x the current count. Period. No control scheme more than that. I'm a big believer ni starting with something simple that we then can refine... > > Yeah, we can start off with something more simple. I picked a lot of constants in here somewhat arbitrarily and it'll take a lot more time tuning those constants than it would take to tune something simpler. > > We do have control over the tick rate and the number of textures to upload though, so maybe we can have both of those depend on the average upload time. > > >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:111 > >> + m_textureUpdatesPerTick = ceil(texturesPerSecond * m_textureUpdateTickPeriod * .25 + epsilon); > > > > I don't like the idea of setting m_textureUpdatesPerTick to less than the value returned by maxPartialTextureUpdates(). We need to allow uploading of maxPartialTextureUpdates textures not to break partial uploads and that would make the timing estimates here incorrect. > > > > To get the most out of increasing m_textureUpdatesPerTick we should also allow more partial updates on the main thread. > > Thanks for pointing that out. I see now that CCLayerTreeHost::requestPartialTextureUpdate() actually uses this information to limit the number of partial updates per frame. > > >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:146 > >> + if (fullUploadCount && (count - fullUploadCount) < queue->partialUploadSize()) > > > > I guess this is to compensate for the fact that m_textureUpdatesPerTick can now be less than maxPartialTextureUpdates. Can we instead adjust maxPartialTextureUpdates so that queue->partialUploadSize() is greater than count? > > As a first pass for this patch, I think it'll be easiest to do what you suggest. > > Will we ever want to have CCLayerTreeHost::requestPartialTextureUpdate() take into account a dynamically changing maxPartialTextureUpdates? I think it makes sense to do so. The value from maxPartialTextureUpdates could be passed to beginFrame. > > >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.h:85 > >> + size_t m_sequentialUpdateCount; > > > > It's not obvious to me what this member variable is for. Maybe add a comment? > > It's supposed to be the number of texture uploads so far since the updateMoreTextures that initiated the first texture upload. It is used to avoid making any adjustments during our first upload since we won't have any feedback yet, which I'm realizing may not necessarily be true now. Will need to rethink it. Also, I think there are some race conditions with vsync ticks that might throw off the way I update this variable. Do we need to be able to adjust upload time estimates during a commit? Maybe things would be simpler if we only adjusted the texture upload estimates in between frames? That's also the only time we can adjust maxPartialTextureUpdates.
> Do we need to be able to adjust upload time estimates during a commit? Maybe things would be simpler if we only adjusted the texture upload estimates in between frames? That's also the only time we can adjust maxPartialTextureUpdates. This sounds like a very good simplification.
Created attachment 163494 [details] Patch
Comment on attachment 163494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163494&action=review Here is an update that addresses most of the comments from the previous patch. Still needs some work. > Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.cpp:128 > +double ThrottledTextureUploader::estimatedPixelsPerSecond() This uses the median now. > Source/WebCore/platform/graphics/chromium/ThrottledTextureUploader.h:30 > +#include <deque> Would like to avoid using two deque classes in the same file, but couldn't figure out how to sort wtf/Deque > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:496 > + m_maxPartialTextureUpdates = std::min(m_settings.maxPartialTextureUpdates, m_proxy->maxPartialTextureUpdates()); m_settings.maxPartialTextureUpdates is now treated as read only. m_proxy->maxPartialTextureUpdates() is the current limit the uploader has communicated to the main thread. m_maxPartialTextureUpdates is the limit we will use for the next frame. > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:39 > +static const size_t pixelsPerTexture = 256 * 256; Still not sure how to do this better... > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:60 > void CCTextureUpdateController::updateTextures(CCResourceProvider* resourceProvider, TextureCopier* copier, TextureUploader* uploader, CCTextureUpdateQueue* queue, size_t count) I restructured the logic here a bit to make sure we don't flush until after we've inserted the query. I had noticed in some traces that the queries were being executed a long time after the actual end of the uploads because of the extra flushes. > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:211 > + (size_t) ceil(texturesPerSecond * m_tickPeriod)); This maintains a minimum update count of 12, to keep the previous behavior for slow systems. Faster systems will upload more textures though. > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:250 > + m_client->setMaxPartialTextureUpdatesOnImplThread(maxPartialTextureUpdates()); This is where the impl thread communicates to the main thread the current maxPartialTextureUpdates. > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:861 > + m_mainThreadProxy->postTask(createCCThreadTask(this, &CCThreadProxy::setMaxPartialTextureUpdates, max)); This is where setMaxPartialTextureUpdates makes the jump to the main thread.
Comment on attachment 163494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163494&action=review This is looking pretty good. A few ideas below. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:496 > + m_maxPartialTextureUpdates = std::min(m_settings.maxPartialTextureUpdates, m_proxy->maxPartialTextureUpdates()); How about we pass the maxPartialTextureUpdates value to CCLayerTreeHost::updateLayers as a parameter instead and remove the CCProxy::maxPartialTextureUpdates() function? > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:-77 > - ASSERT(queue->partialUploadSize() <= count); Could we keep this assert in some form? It's useful in preventing us from introducing a partial update regression in this or a future patch. > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:199 > +void CCTextureUpdateController::updateTextureCountAndTickPeriod(double timeBeforeVsync) I think this looks more complicated than it is. Could it somehow be restructured to early out instead of having this tree of if/else branches? Maybe it makes more sense to have a "size_t nextTextureUpdateCount(double timeBeforeVSync)" function instead? Also how is e.g. 1 full update + 11 partial updates handled? That's currently all done in one 4ms update interval but it looks like this patch might be doing it less efficiently. Or am I missing something? > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.h:41 > + virtual void setMaxPartialTextureUpdatesOnImplThread(size_t) = 0; How about getting maxPartialTextureUpdates from the uploader instead? It seems like all the state needed to compute that is already maintained there. Btw, I don't think this function should have "OnImplThread" in the name, that's a CCThreadProxy specific detail. > Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.h:80 > + double m_tickPeriod; You don't seem to be using this. > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:861 > + m_mainThreadProxy->postTask(createCCThreadTask(this, &CCThreadProxy::setMaxPartialTextureUpdates, max)); Would it be easier to pass this value to the main thread with CCThreadProxy::beginFrame? Changing it at a different time shouldn't be allowed anyhow.
Comment on attachment 163494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=163494&action=review >>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:496 >>> + m_maxPartialTextureUpdates = std::min(m_settings.maxPartialTextureUpdates, m_proxy->maxPartialTextureUpdates()); >> >> m_settings.maxPartialTextureUpdates is now treated as read only. >> m_proxy->maxPartialTextureUpdates() is the current limit the uploader has communicated to the main thread. >> m_maxPartialTextureUpdates is the limit we will use for the next frame. > > How about we pass the maxPartialTextureUpdates value to CCLayerTreeHost::updateLayers as a parameter instead and remove the CCProxy::maxPartialTextureUpdates() function? I like. >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:-77 >> - ASSERT(queue->partialUploadSize() <= count); > > Could we keep this assert in some form? It's useful in preventing us from introducing a partial update regression in this or a future patch. We can keep it if we synchronize maxPartialTextureUpdates on both the main and impl threads. This patch doesn't synchronize them and I'm not sure if it'll be more complicated than it's worth yet. >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:199 >> +void CCTextureUpdateController::updateTextureCountAndTickPeriod(double timeBeforeVsync) > > I think this looks more complicated than it is. Could it somehow be restructured to early out instead of having this tree of if/else branches? Maybe it makes more sense to have a "size_t nextTextureUpdateCount(double timeBeforeVSync)" function instead? > > Also how is e.g. 1 full update + 11 partial updates handled? That's currently all done in one 4ms update interval but it looks like this patch might be doing it less efficiently. Or am I missing something? >I think this looks more complicated than it is. Will restructure and rename. > Also how is e.g. 1 full update + 11 partial updates handled? Good catch. You are correct, although that was not my intent. This function will still set m_textureUpdatesPerTick to 12, however I need to fix CCTextureUpdateController::updateTextures so it doesn't early out. >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.h:41 >> + virtual void setMaxPartialTextureUpdatesOnImplThread(size_t) = 0; > > How about getting maxPartialTextureUpdates from the uploader instead? It seems like all the state needed to compute that is already maintained there. Btw, I don't think this function should have "OnImplThread" in the name, that's a CCThreadProxy specific detail. I put this function here instead of in the uploader because, although the uploader had all the state to compute the bandwidth, the CCTextureUpdateController had the state to know *when* to compute the bandwidth. Will change the when to be in CCThreadProxy::scheduledActionBeginFrame. Will remove OnImplThread suffix. >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.h:80 >> + double m_tickPeriod; > > You don't seem to be using this. It is used but isn't really a variable anymore, so I will remove it. >>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:861 >>> + m_mainThreadProxy->postTask(createCCThreadTask(this, &CCThreadProxy::setMaxPartialTextureUpdates, max)); >> >> This is where setMaxPartialTextureUpdates makes the jump to the main thread. > > Would it be easier to pass this value to the main thread with CCThreadProxy::beginFrame? Changing it at a different time shouldn't be allowed anyhow. I initially tried to do that, but it seemed more complicated than it's worth. Upon further inspection, it doesn't look too bad.
(In reply to comment #13) > (From update of attachment 163494 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163494&action=review > > >>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:496 > >>> + m_maxPartialTextureUpdates = std::min(m_settings.maxPartialTextureUpdates, m_proxy->maxPartialTextureUpdates()); > >> > >> m_settings.maxPartialTextureUpdates is now treated as read only. > >> m_proxy->maxPartialTextureUpdates() is the current limit the uploader has communicated to the main thread. > >> m_maxPartialTextureUpdates is the limit we will use for the next frame. > > > > How about we pass the maxPartialTextureUpdates value to CCLayerTreeHost::updateLayers as a parameter instead and remove the CCProxy::maxPartialTextureUpdates() function? > > I like. > > >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:-77 > >> - ASSERT(queue->partialUploadSize() <= count); > > > > Could we keep this assert in some form? It's useful in preventing us from introducing a partial update regression in this or a future patch. > > We can keep it if we synchronize maxPartialTextureUpdates on both the main and impl threads. This patch doesn't synchronize them and I'm not sure if it'll be more complicated than it's worth yet. Hm, when processing partial updates the impl thread need to be passing "count >= queue->partialUploadSize()" as partial updates can't be split into two groups. What's the case when this ASSERT is causing problems? I don't think I understand why it can't just be left as is. > > >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.cpp:199 > >> +void CCTextureUpdateController::updateTextureCountAndTickPeriod(double timeBeforeVsync) > > > > I think this looks more complicated than it is. Could it somehow be restructured to early out instead of having this tree of if/else branches? Maybe it makes more sense to have a "size_t nextTextureUpdateCount(double timeBeforeVSync)" function instead? > > > > Also how is e.g. 1 full update + 11 partial updates handled? That's currently all done in one 4ms update interval but it looks like this patch might be doing it less efficiently. Or am I missing something? > > >I think this looks more complicated than it is. > Will restructure and rename. > > > Also how is e.g. 1 full update + 11 partial updates handled? > Good catch. You are correct, although that was not my intent. This function will still set m_textureUpdatesPerTick to 12, however I need to fix CCTextureUpdateController::updateTextures so it doesn't early out. > > >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.h:41 > >> + virtual void setMaxPartialTextureUpdatesOnImplThread(size_t) = 0; > > > > How about getting maxPartialTextureUpdates from the uploader instead? It seems like all the state needed to compute that is already maintained there. Btw, I don't think this function should have "OnImplThread" in the name, that's a CCThreadProxy specific detail. > > I put this function here instead of in the uploader because, although the uploader had all the state to compute the bandwidth, the CCTextureUpdateController had the state to know *when* to compute the bandwidth. > > Will change the when to be in CCThreadProxy::scheduledActionBeginFrame. > Will remove OnImplThread suffix. > > >> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdateController.h:80 > >> + double m_tickPeriod; > > > > You don't seem to be using this. > > It is used but isn't really a variable anymore, so I will remove it. > > >>> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:861 > >>> + m_mainThreadProxy->postTask(createCCThreadTask(this, &CCThreadProxy::setMaxPartialTextureUpdates, max)); > >> > >> This is where setMaxPartialTextureUpdates makes the jump to the main thread. > > > > Would it be easier to pass this value to the main thread with CCThreadProxy::beginFrame? Changing it at a different time shouldn't be allowed anyhow. > > I initially tried to do that, but it seemed more complicated than it's worth. Upon further inspection, it doesn't look too bad. I think it might be cleaner. On a more general note, I'm not fully convinced that adjusting the number of texture updates per tick is better than adjusting the tick rate. Both approaches have pros and cons. Before we get into too much detail, can we add some details about the two different methods and some rational for picking one over the other to the texture upload strategy doc?
Moving review to https://codereview.chromium.org/10916292 because of GTFO.
I don't think this bug is relevant anymore.