Bug 73454

Created attachment 119277 [details] Patch

Comment on attachment 119277 [details] Patch This looks like something Nat should review.

Comment on attachment 119277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119277&action=review Thanks so much for poking at this! A bunch of food for thought, let me know how you want to proceed. I'll get the patch to unofficial LGTM stage and then JamesR will nail you to the wall with a strict review. ;) > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:88 > + m_beginTimeHistoryInSec[FRAME_INDEX(m_currentFrameNumber - 1)]; /me can't remember, -1 % N is still -1, right? Point being, wraparound might be wrong, if my memory is right at least. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:89 > + m_badFrame[FRAME_INDEX(m_currentFrameNumber)] = (delta < 1.0 / 70) || (delta > 5.0 / 60); When we have vsync turned off, eg --disable-gpu-vsync, this would mean frame rate peaks at 70fps, right? We're going to be getting rid of these fast frames over time, so I'd rather get correct numbers for very fast framerates... thoughts? > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:204 > // Filter the frame times to avoid spikes. Might want to explain the filtering algorithm here sort of how you did in the changelog. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:210 > + // don't filter the time if this is the first frame after a period of no drawing; pretend this don't -> Don't. Complete sentences for comments throughout the file. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:216 > + if (secForThirdLastFrame > 0.5 || m_averageFPSCount > 60*2) { Should we pull the various constants to kBlahBlahBlah? What is this check adding over the m_badFrame[FRAME_NUMBER(m_cur - 3)] check? I'm sure its something, I just can't figure out what it is in my head. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:230 > + Hey, crazy question? Its probably ~100 frames history that we keep right? That should be like stupid fast to compute an average for... we only run this when they ask for it, so paying a few usec for this is totally fine by me. The reason I ask is because this code is getting a bit hard to read. Not your fault... its just we're doing a few different things here. Thats not bad, but suppose down the road we decide instead of stddev, we need a metric like "number of frames that we dropped." At that point, I'd love it if the code was really easy to hack on. If we just go to "compute everything every time from a frame history", does the code get cleaner? I personally like having less state and state overlap in that case.... > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:233 > + String text(String::format("FPS: XXX.X (XX.X XXX.XXX)")); Is there a way to re-use the format string here? > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:236 > + text = String::format("FPS: %03.01f (%02.01f %03.03f)", 1.0 / m_filteredFrameTime, m_averageFPS, stdDeviation); I'm not sure the average person from the street is going to understand the variance vs average, much less stddev. I also have thrown around the idea of reporting late "% frames we dropped" instead of variance at all, since stddev is really funky for fps (2ms variance on 60fps is 60 fps+/- something strange). What if we report out filtered fps and let people see variance from the graph? I'm all for leaving calculations in place because we've had requests for a follow-on patch that exposes frame rate metrics to javascript for people who have test harnesses/etc. Within that context, I imagine we could expose a few different values... > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:278 > + if (!m_badFrame[j]) { Might want to explain what this block of code does. It kind of breaks my head > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:290 > + if (minimumOutlier != noOutlierFound) { What's the value add of showing the outlier?

Comment on attachment 119277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119277&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:88 >> + m_beginTimeHistoryInSec[FRAME_INDEX(m_currentFrameNumber - 1)]; > > /me can't remember, -1 % N is still -1, right? Point being, wraparound might be wrong, if my memory is right at least. You're absolutely right. Originally I'd had the macro as ((frame) + kBegin)%kBegin but I was looking at that thinking this extra kBegin doesn't look meaningful and I broke it just prior to review while going through my own diff. How embarrassing :( I'll fix the macro again. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:89 >> + m_badFrame[FRAME_INDEX(m_currentFrameNumber)] = (delta < 1.0 / 70) || (delta > 5.0 / 60); > > When we have vsync turned off, eg --disable-gpu-vsync, this would mean frame rate peaks at 70fps, right? We're going to be getting rid of these fast frames over time, so I'd rather get correct numbers for very fast framerates... thoughts? No, the effect of this will be that if you have --disable-gpu-vsync the FPS meter will refuse to show you data because the data are wrong. I preferred to have an empty graph rather than show the user false frame rates - we are painting frames the user will never see in this case. In addition this filtering is ultimately what detects the spike due to startup after a pause, since the early swapbuffers calls are nonblocking and all get assigned silly high frame rates (like 110FPS). How critical is seeing the bad data for the no vsync case? I'm open to suggestions. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:204 >> // Filter the frame times to avoid spikes. > > Might want to explain the filtering algorithm here sort of how you did in the changelog. Happy to do so but this is the pre-existing code. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:210 >> + // don't filter the time if this is the first frame after a period of no drawing; pretend this > > don't -> Don't. > Complete sentences for comments throughout the file. Sorry will fix, not used to webkit style. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:216 >> + if (secForThirdLastFrame > 0.5 || m_averageFPSCount > 60*2) { > > Should we pull the various constants to kBlahBlahBlah? > > What is this check adding over the m_badFrame[FRAME_NUMBER(m_cur - 3)] check? I'm sure its something, I just can't figure out what it is in my head. Bad frames can occur within the sample. So if you stop doing anything and then you start to scroll, you'll get a few bad frames at the start of the sample, and m_badFrame lets you skip those so they don't mess up the average and standard deviation; while this check determines that since the third last frame was so very long (> 0.5s, or whatever threshold we choose) we should start a new set of samples for timing the animation. Deciding if something is a bad frame or not is a separate set of heuristics/constants than deciding how long a pause is needed to consider the sample data a new stream of frames for a new animation. The second half of the if is ensuring that we don't take samples longer than 2s; the assumption is that in a long running animation, you'd prefer to see problems with variance in a reasonably small window, whereas if you accumulated thousands of datapoints the outliers wouldn't be able to move the average of the variance and that would hide problems. So if you run something like poster circle the stats keep resetting and glitches impact the variance quite a bit. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:230 >> + > > Hey, crazy question? Its probably ~100 frames history that we keep right? That should be like stupid fast to compute an average for... we only run this when they ask for it, so paying a few usec for this is totally fine by me. > > The reason I ask is because this code is getting a bit hard to read. Not your fault... its just we're doing a few different things here. Thats not bad, but suppose down the road we decide instead of stddev, we need a metric like "number of frames that we dropped." At that point, I'd love it if the code was really easy to hack on. > > If we just go to "compute everything every time from a frame history", does the code get cleaner? I personally like having less state and state overlap in that case.... We are only keeping 64 data points. We could loop over them and compute every time, and I don't think it would be ridiculously inefficient though it depends on the platform (on ARM, computing this from scratch might matter). I wasn't primarily worried about computation time here but rather about not introducing floating point errors. I don't know of a technique for computing these metrics that is both simple and correct - do you have a reference? If I was to take your suggestion I'd just put this existing code in a loop which wouldn't really help with the complexity of this block. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:233 >> + String text(String::format("FPS: XXX.X (XX.X XXX.XXX)")); > > Is there a way to re-use the format string here? I was using the wrong format string, I can change it to %5.1f (%4.1f %7.1f) to get the effect, will do. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:236 >> + text = String::format("FPS: %03.01f (%02.01f %03.03f)", 1.0 / m_filteredFrameTime, m_averageFPS, stdDeviation); > > I'm not sure the average person from the street is going to understand the variance vs average, much less stddev. I also have thrown around the idea of reporting late "% frames we dropped" instead of variance at all, since stddev is really funky for fps (2ms variance on 60fps is 60 fps+/- something strange). > > What if we report out filtered fps and let people see variance from the graph? I'm all for leaving calculations in place because we've had requests for a follow-on patch that exposes frame rate metrics to javascript for people who have test harnesses/etc. Within that context, I imagine we could expose a few different values... The variance reported is in FPS, so the numbers are average +/- multiple of stddev. I really need the stddev to do anything statistically meaningful. What about making it verbose like X FPS, Y% of values in {LOW, HIGH} s = stddev? >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:278 >> + if (!m_badFrame[j]) { > > Might want to explain what this block of code does. It kind of breaks my head I'd rather reorder it to be clearer. Is it the inner if that's the problem? or the whole block? >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:290 >> + if (minimumOutlier != noOutlierFound) { > > What's the value add of showing the outlier? Easiest to see if you run the code, since there's no scale on the graph it's nice to see the little 30.0 floating under the downspike as it scrolls by in the graph. Could remove, but helps you see how bad the bad frames are.

Comment on attachment 119277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119277&action=review >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:89 >>> + m_badFrame[FRAME_INDEX(m_currentFrameNumber)] = (delta < 1.0 / 70) || (delta > 5.0 / 60); >> >> When we have vsync turned off, eg --disable-gpu-vsync, this would mean frame rate peaks at 70fps, right? We're going to be getting rid of these fast frames over time, so I'd rather get correct numbers for very fast framerates... thoughts? > > No, the effect of this will be that if you have --disable-gpu-vsync the FPS meter will refuse to show you data because the data are wrong. I preferred to have an empty graph rather than show the user false frame rates - we are painting frames the user will never see in this case. In addition this filtering is ultimately what detects the spike due to startup after a pause, since the early swapbuffers calls are nonblocking and all get assigned silly high frame rates (like 110FPS). > > How critical is seeing the bad data for the no vsync case? I'm open to suggestions. I guess we have disagreeing interpretations of frame rate. I had taken this counter to indicate *chrome* rendering FPS, not window manager FPS --- if you want that, people tend to use their window manager's FPS counter [on OSX and Windows, there are tools to measure that]. On aura, we'd turn on FPS counter int he browser compositor :) If we are swapping at 110fps, thats actually a meaningful value, its just a matter of perspective. I agree that a user might say "i see 110 but i've got a camera pointed at my screen and see 60." However, people doing rendering optimizations run with vsync off in order to get that 110 fps number: it indicates the peak rate that you could run at, and shows how gpu-bound you are. E.g. danakj's culling work impact on GPU perf can be measured from FPS without vsync... Anyway, food for thought. What do you think with some of that data in mind? >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:204 >>> // Filter the frame times to avoid spikes. >> >> Might want to explain the filtering algorithm here sort of how you did in the changelog. > > Happy to do so but this is the pre-existing code. Yeah, sorry! Darn those commons! :) >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:210 >>> + // don't filter the time if this is the first frame after a period of no drawing; pretend this >> >> don't -> Don't. >> Complete sentences for comments throughout the file. > > Sorry will fix, not used to webkit style. it breaks my head too. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:214 > m_filteredFrameTime = ((1.0 - alpha) * m_filteredFrameTime) + (alpha * secForLastFrame); Is there a reason we don't don't reset the filtered frame time for long frames? >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:216 >>> + if (secForThirdLastFrame > 0.5 || m_averageFPSCount > 60*2) { >> >> Should we pull the various constants to kBlahBlahBlah? >> >> What is this check adding over the m_badFrame[FRAME_NUMBER(m_cur - 3)] check? I'm sure its something, I just can't figure out what it is in my head. > > Bad frames can occur within the sample. So if you stop doing anything and then you start to scroll, you'll get a few bad frames at the start of the sample, and m_badFrame lets you skip those so they don't mess up the average and standard deviation; while this check determines that since the third last frame was so very long (> 0.5s, or whatever threshold we choose) we should start a new set of samples for timing the animation. Deciding if something is a bad frame or not is a separate set of heuristics/constants than deciding how long a pause is needed to consider the sample data a new stream of frames for a new animation. > > The second half of the if is ensuring that we don't take samples longer than 2s; the assumption is that in a long running animation, you'd prefer to see problems with variance in a reasonably small window, whereas if you accumulated thousands of datapoints the outliers wouldn't be able to move the average of the variance and that would hide problems. So if you run something like poster circle the stats keep resetting and glitches impact the variance quite a bit. Ah.... so a "bad frame" doesn't count against the average. A long frame resets the average. Is that a fair summary? But why do we reset the fps every 120 samples? Can we just drop expontentially weighted frame time and use the average and stddev only? This reset logic fixes the problem the expontential weighting was introduced for --- chiefly, making it converge on something sane with big pauses sometimes... > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:231 > // Create & measure FPS text. Been tuning android all day this week, I'm not worried about cpu time... we spend a good half millisecond putting this data into textures, then to the gpu process, in a very inefficient way. The calculation is probably epsilon on that. If we go to stddev and average as the only metric, then isn't it just a matter of walking [with wraparound] from the oldest frame to the newest frame doing the calculations above as you go? The nice thing is that we could make this into a getFPSAndStdDev(double& fps, double& stddev) const, where our only member vars are m_frameBeginTime and maaaayybe of m_badFrame... >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:236 >>> + text = String::format("FPS: %03.01f (%02.01f %03.03f)", 1.0 / m_filteredFrameTime, m_averageFPS, stdDeviation); >> >> I'm not sure the average person from the street is going to understand the variance vs average, much less stddev. I also have thrown around the idea of reporting late "% frames we dropped" instead of variance at all, since stddev is really funky for fps (2ms variance on 60fps is 60 fps+/- something strange). >> >> What if we report out filtered fps and let people see variance from the graph? I'm all for leaving calculations in place because we've had requests for a follow-on patch that exposes frame rate metrics to javascript for people who have test harnesses/etc. Within that context, I imagine we could expose a few different values... > > The variance reported is in FPS, so the numbers are average +/- multiple of stddev. I really need the stddev to do anything statistically meaningful. > What about making it verbose like X FPS, Y% of values in {LOW, HIGH} s = stddev? If we go to just average, then I'm down with whatever you think is the clearest way to present it. Lets be kind to people who leave this onscreen and not eat the entire top part of the screen, of course. :) >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:278 >>> + if (!m_badFrame[j]) { >> >> Might want to explain what this block of code does. It kind of breaks my head > > I'd rather reorder it to be clearer. Is it the inner if that's the problem? or the whole block? 278-287 was really hard to read. If you can reorder it, that's cool. Or just some docs... That 5-part if statement on fps is what really was hard. Temp vars maybe? >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:290 >>> + if (minimumOutlier != noOutlierFound) { >> >> What's the value add of showing the outlier? > > Easiest to see if you run the code, since there's no scale on the graph it's nice to see the little 30.0 floating under the downspike as it scrolls by in the graph. Could remove, but helps you see how bad the bad frames are. I think I get it. This is a cool idea!

Created attachment 124885 [details] Patch

Nat, I think I made all the changes we'd discussed save one: I couldn't find the boolean to make ignoring the fast frames conditional on whether or not the scheduler is turned on. Sorry can you remind me where that flag is? I thought I'd post this anyway because you might want to give it another once over looking for problems. I removed the outlier code which was just difficult to make easy on the eyes, as well as the EMA code that isn't really needed any more so I think overall the patch looks a little nicer/smaller. PTAL and let me know what more we need to do to get this landed. I'll be faster than a month for this next iteration :-)

Comment on attachment 124885 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124885&action=review To test for non-threaded mode, which is the one that does the doubleframe thing, bool bSchedulerDoesDoubleframes = !CCProxy::hasImplThread(). Have to catch a bus. Will look more if it has wifi. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:82 > +// works on -1 Nice. Webkit Style devil say comments are complete sentences. :) > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:91 > + m_badFrame[FRAME_INDEX(m_currentFrameNumber)] = (delta < kFrameTooFast) || (delta > kFrameTooSlow); Can't we reconstruct this at draw time rather than storing it? I'd love to avoid redundant state.

Comment on attachment 124885 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124885&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:82 >> +// works on -1 > > Nice. Webkit Style devil say comments are complete sentences. :) fixed >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:91 >> + m_badFrame[FRAME_INDEX(m_currentFrameNumber)] = (delta < kFrameTooFast) || (delta > kFrameTooSlow); > > Can't we reconstruct this at draw time rather than storing it? I'd love to avoid redundant state. I did this as you asked (see the upcoming patch) but I wouldn't normally make the tradeoff this way. If I can trade a constant thing for an O(n) thing, I take the constant thing; in this case, it's constant space versus O(n) time to recompute for every call; constant space would be the winner for me (as the HUD is essentially a singleton). If it was O(n) space versus O(n) time it'd depend on how many objects, and if it was O(n) space versus constant time I'd take recomputation. In this instance I really think it makes no difference as the computation is small, so I just switched it out. I just don't think anyone would ever notice the difference either way in this example.

Created attachment 125159 [details] Patch

Comment on attachment 125159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125159&action=review Thanks so much! I think if we can ditch the kLongAnimationTriggersReset, I'd be happy with this. I think we will need to do a followup patch that improves the numerical stability of the FPS average for long running apps that are always animating. For that case, we'd never get a FPS reset, and so the FPS and Count values would constantly grow. It seems to me that we have a history value lying around and the awesome new FRAME_INDEX() macro. In the followup patch, I think we can simply make a function that is getAverageFPSAndVariance() that walks back from the current frame number until a long frame is reached or we wrap around to the start point. Once the "gesture start index" is found, we walk forward through the timestamps to the current frame number, running your existing FPS accumulation logic and discarding "bad" frames. Voila, stable FPS. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:54 > + , m_varianceNumerator(0) Under the heading of tormenting you to death: m_averageFPSSampleCount m_fpsVarianceNumerator [or m_frameDurationNumerator, depending on whether the variance is 1/x or x] > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:209 > + double secForThirdLastFrame = m_beginTimeHistoryInSec[FRAME_INDEX(m_currentFrameNumber - 3)] - might want a comment explaining why 3rd and not 2nd. Iirw, its because of doubleframes, right? > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:216 > + // the scheduler is not active in order to represent the true frame rate in scheduler is not active -> the single threaded scheduler is active something like that. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:219 > + if (secForThirdLastFrame > kIdleSecondsTriggersReset || m_averageFPSCount > kLongAnimationTriggersReset) { Shouldn't the isBadFrame()'s low value being set to say, 10fps, eliminate these frames entirely? I'd prefer that because then we have a proper running average, rather than an average of the last 120 frames. The 120-frame average bugs me because for a sample(i), the amount of "support" that it has is based on i%120. E.g. sample 0 is going to have zero support and be exaclty that value, whereas sample 127 is going to be highly filtered by the preecindg samples. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:267 > + if (!isBadFrame(j)) { Might be cleaner if you say if(isBadFrame(j)) continue; > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:89 > + static const double kFrameTooFast = 1.0 / 70; I'm thinking 1/150 might be better here. 6.6ms will still catch the doubleframes, but will give us better protection against people with fancy 120hz monitors. Might put the units on this. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:90 > + static const double kFrameTooSlow = 5.0 / 60; Can we fram this instead as 1 / 12? That way its more obviously 12fps Units.

Comment on attachment 125159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125159&action=review > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:84 > +#define FRAME_INDEX(frame) SAFE_MOD(frame, kBeginFrameHistorySize) does this really have to be a macro? i think this would be much better as a simple static function so we could get useful type checking and easier debugging > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:175 > + bool bSchedulerAllowsDoubleFrames = !CCProxy::hasImplThread(); webkit style is for not prefix variable names with a "b" or other suffix like this. I think schedulerAllowsDoubleFrames would be fine > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:237 > + float textWidth = m_mediumFont->width(text) + 2.0f; just "+ 2;". See http://www.webkit.org/coding/coding-style.html#float-suffixes >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:267 >> + if (!isBadFrame(j)) { > > Might be cleaner if you say > if(isBadFrame(j)) > continue; agree, especially since we use 4-space indents in WebKit it's good to use early outs instead of nesting to try to keep indentation under control and avoid unnecessary churn on lines > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:268 > + double p = 1 - ((fps - loFPS) / (hiFPS - loFPS)); use a full word. other than loop counters or x/y/z for coordinates we pretty much never use one-letter variable names in WebKit. I realize that this file in particular has a lot of style violations - I was pretty lax when initially reviewing it - but it's good to get into good habits in order to keep the codebase consistent where possible > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:84 > + static const double kIdleSecondsTriggersReset = 0.5; constants should not have the 'k' prefix (although I realize that other constants here are incorrectly named) - just name them like normal variables. I can't tell why these are in the header and not simply statics in the .cpp

Comment on attachment 125159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125159&action=review >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:54 >> + , m_varianceNumerator(0) > > Under the heading of tormenting you to death: > > m_averageFPSSampleCount > m_fpsVarianceNumerator [or m_frameDurationNumerator, depending on whether the variance is 1/x or x] Fixed. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:84 >> +#define FRAME_INDEX(frame) SAFE_MOD(frame, kBeginFrameHistorySize) > > does this really have to be a macro? i think this would be much better as a simple static function so we could get useful type checking and easier debugging I don't really care, will switch. No useful type checking to do here, static member has to be a class member for access to kBeginFrameHistorySize. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:175 >> + bool bSchedulerAllowsDoubleFrames = !CCProxy::hasImplThread(); > > webkit style is for not prefix variable names with a "b" or other suffix like this. I think schedulerAllowsDoubleFrames would be fine Name taken directly from nduca's review, will change. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:209 >> + double secForThirdLastFrame = m_beginTimeHistoryInSec[FRAME_INDEX(m_currentFrameNumber - 3)] - > > might want a comment explaining why 3rd and not 2nd. Iirw, its because of doubleframes, right? Hmm. I will look into this and explain or modify, in this current version of the patch I see no need for this. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:216 >> + // the scheduler is not active in order to represent the true frame rate in > > scheduler is not active -> the single threaded scheduler is active > > something like that. OK will fix. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:219 >> + if (secForThirdLastFrame > kIdleSecondsTriggersReset || m_averageFPSCount > kLongAnimationTriggersReset) { > > Shouldn't the isBadFrame()'s low value being set to say, 10fps, eliminate these frames entirely? I'd prefer that because then we have a proper running average, rather than an average of the last 120 frames. The 120-frame average bugs me because for a sample(i), the amount of "support" that it has is based on i%120. E.g. sample 0 is going to have zero support and be exaclty that value, whereas sample 127 is going to be highly filtered by the preecindg samples. To get a true moving average we'd have to recompute on every cycle. We could just set kLongAnimationTriggersReset to a much bigger value? But that defeats the purpose of the reset, since its goal is to expose jank. If we really need this gone, I'd rather recompute every time as you've suggested in your overview comment. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:237 >> + float textWidth = m_mediumFont->width(text) + 2.0f; > > just "+ 2;". See http://www.webkit.org/coding/coding-style.html#float-suffixes OK will fix. >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:267 >>> + if (!isBadFrame(j)) { >> >> Might be cleaner if you say >> if(isBadFrame(j)) >> continue; > > agree, especially since we use 4-space indents in WebKit it's good to use early outs instead of nesting to try to keep indentation under control and avoid unnecessary churn on lines This means repeating the line x += 1 from 240/278 here. I don't care enough to argue the point, but maybe just confirm that's what you want? >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:268 >> + double p = 1 - ((fps - loFPS) / (hiFPS - loFPS)); > > use a full word. other than loop counters or x/y/z for coordinates we pretty much never use one-letter variable names in WebKit. I realize that this file in particular has a lot of style violations - I was pretty lax when initially reviewing it - but it's good to get into good habits in order to keep the codebase consistent where possible This is a pre-existing variable name from 231 of the original code. Can I do a separate renaming/refactoring patch to fix up the name and then apply this patch to get the logic, or can I follow this patch with a renaming patch? >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:84 >> + static const double kIdleSecondsTriggersReset = 0.5; > > constants should not have the 'k' prefix (although I realize that other constants here are incorrectly named) - just name them like normal variables. > > I can't tell why these are in the header and not simply statics in the .cpp Pure case of monkey-see, monkey-do. Again, I'd prefer to make these changes in a refactoring patch to land before or after this one to avoid muddying the waters. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:89 >> + static const double kFrameTooFast = 1.0 / 70; > > I'm thinking 1/150 might be better here. 6.6ms will still catch the doubleframes, but will give us better protection against people with fancy 120hz monitors. > > Might put the units on this. I'll try it and take the change if it works. This also will make the graph 50% taller. >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:90 >> + static const double kFrameTooSlow = 5.0 / 60; > > Can we fram this instead as 1 / 12? That way its more obviously 12fps > > Units. I thought it was more intuitive to think of it as 5 missed frames than think of it as slower than 12 fps, but really don't care either way. You ask for units, do you care about the naming? kFrameTooSlowMS?

(In reply to comment #13) > Name taken directly from nduca's review, will change. sorry. I think jamesr, we can let the 'x' and 'p' slide. This code is non normative due to me screwing up previously. I think its perfectly acceptable to finish this patch and file a followup bug about making this file more normative. > To get a true moving average we'd have to recompute on every cycle. We could just set kLongAnimationTriggersReset to a much bigger value? But that defeats the purpose of the reset, since its goal is to expose jank. If we really need this gone, I'd rather recompute every time as you've suggested in your overview comment. Yeah, lets get a true windowed average. If one-off spikes prove to be an issue once that is done, computing-every-frame gives us a clean way to then switch from an average to median or 75th percentile. > This means repeating the line x += 1 from 240/278 here. I don't care enough to argue the point, but maybe just confirm that's what you want? Go for it. :) > > >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:268 > >> + double p = 1 - ((fps - loFPS) / (hiFPS - loFPS)); > > > > use a full word. other than loop counters or x/y/z for coordinates we pretty much never use one-letter variable names in WebKit. I realize that this file in particular has a lot of style violations - I was pretty lax when initially reviewing it - but it's good to get into good habits in order to keep the codebase consistent where possible > > This is a pre-existing variable name from 231 of the original code. Can I do a separate renaming/refactoring patch to fix up the name and then apply this patch to get the logic, or can I follow this patch with a renaming patch? I think fixing this is beyond the scope of this patch. File a bug, assign to me if you want. I'm hoping james will be ok with this compromise. > I'll try it and take the change if it works. This also will make the graph 50% taller. Oh, hm. Lets just decouple the graph dimensions then. I think your graph sizing was well chosen --- there's little reason the two have to be coupled. > >> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:90 > >> + static const double kFrameTooSlow = 5.0 / 60; > > > > Can we fram this instead as 1 / 12? That way its more obviously 12fps > I thought it was more intuitive to think of it as 5 missed frames than think of it as slower than 12 fps, but really don't care either way. You ask for units, do you care about the naming? kFrameTooSlowMS? Ah, I understand your rationale. But for that, then i'd name the constant numMissedFramesBeforeReset or something like that and do the conversion to ms on the fly.

Comment on attachment 125159 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125159&action=review >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:84 >>> +#define FRAME_INDEX(frame) SAFE_MOD(frame, kBeginFrameHistorySize) >> >> does this really have to be a macro? i think this would be much better as a simple static function so we could get useful type checking and easier debugging > > I don't really care, will switch. No useful type checking to do here, static member has to be a class member for access to kBeginFrameHistorySize. Done >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:175 >>> + bool bSchedulerAllowsDoubleFrames = !CCProxy::hasImplThread(); >> >> webkit style is for not prefix variable names with a "b" or other suffix like this. I think schedulerAllowsDoubleFrames would be fine > > Name taken directly from nduca's review, will change. Done. >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:209 >>> + double secForThirdLastFrame = m_beginTimeHistoryInSec[FRAME_INDEX(m_currentFrameNumber - 3)] - >> >> might want a comment explaining why 3rd and not 2nd. Iirw, its because of doubleframes, right? > > Hmm. I will look into this and explain or modify, in this current version of the patch I see no need for this. This is not necessary now (I will be kind to myself and assume in the first iteration it was). I changed it to use the second last. >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:216 >>> + // the scheduler is not active in order to represent the true frame rate in >> >> scheduler is not active -> the single threaded scheduler is active >> >> something like that. > > OK will fix. Done >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:237 >>> + float textWidth = m_mediumFont->width(text) + 2.0f; >> >> just "+ 2;". See http://www.webkit.org/coding/coding-style.html#float-suffixes > > OK will fix. Done >>>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:267 >>>> + if (!isBadFrame(j)) { >>> >>> Might be cleaner if you say >>> if(isBadFrame(j)) >>> continue; >> >> agree, especially since we use 4-space indents in WebKit it's good to use early outs instead of nesting to try to keep indentation under control and avoid unnecessary churn on lines > > This means repeating the line x += 1 from 240/278 here. I don't care enough to argue the point, but maybe just confirm that's what you want? Done. >>> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:89 >>> + static const double kFrameTooFast = 1.0 / 70; >> >> I'm thinking 1/150 might be better here. 6.6ms will still catch the doubleframes, but will give us better protection against people with fancy 120hz monitors. >> >> Might put the units on this. > > I'll try it and take the change if it works. This also will make the graph 50% taller. This doesn't work well for me. On debug builds on my Z600 I get spikes up to about 78-80FPS regularly that are fake.

Created attachment 130531 [details] Patch

Created attachment 130539 [details] Patch

The latest patch has all the proposed changes except renamings which I'd still be happy to do in a follow-up or prereq patch. The second from last patch has all changes except factoring out the average to be a moving average so that trivial responses to comments can be seen without the code being completely rearranged. You can ignore it or use it to tick off old comments.

Comment on attachment 130539 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=130539&action=review LGTM with nits. Thanks again for putting up with us. :) You'll probably need to rebase as james just landed some very minor changes this morning that fix the HUD for threaded mode. > Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:208 > +void CCHeadsUpDisplay::getAverageFPSAndStandardDeviation(double *average, double *standardDeviation) const Little style nit. /me wonders if we should just make a little struct on CCHeadsUpDisplay so we can avoid passing by pointer, then convering to reference awkwardness... struct FrameStats { avg stdev } FrameRate getFrameStats() ... Or, you could just keep as is, but make double& averageFPS ==> double averageFPS and assign to the out value only at the end...

Comment on attachment 130539 [details] Patch Looks good but you are going to have merge conflicts due to http://trac.webkit.org/changeset/113081 (sorry!). Please rebase and consider Nat's feedback.

Created attachment 136629 [details] Patch

Patch in previous upload (https://bugs.webkit.org/show_bug.cgi?id=73454#c21) is teh patch as it builds and tests against gclient just prior to the upload. I will in a few minutes upload the patch against tot.

Created attachment 136634 [details] Patch

The latest patch builds and runs on tot.

Awesome. @jamesr for final review and cq.

Comment on attachment 136634 [details] Patch OK!

Comment on attachment 136634 [details] Patch Clearing flags on attachment: 136634 Committed r113872: <http://trac.webkit.org/changeset/113872>

All reviewed patches have been landed. Closing bug.

This is being rolled out. It broke the mac and windows builds: 3>c:\b\build\slave\webkit-win-latest-rel\build\src\third_party\webkit\source\webcore\platform\graphics\chromium\cc/CCHeadsUpDisplay.h(87) : error C2864: 'WebCore::CCHeadsUpDisplay::kIdleSecondsTriggersReset' : only static const integral data members can be initialized within a class 3>c:\b\build\slave\webkit-win-latest-rel\build\src\third_party\webkit\source\webcore\platform\graphics\chromium\cc/CCHeadsUpDisplay.h(89) : error C2864: 'WebCore::CCHeadsUpDisplay::kFrameTooFast' : only static const integral data members can be initialized within a class

Created attachment 136696 [details] Patch

The new patch should fix the windows compile but I haven't verified it on Windows yet. It does still build and work on linux and does resolve the error from the compiler.

I validated the patch builds and the --show-fps-counter flag works on Windows by testing manually.

Comment on attachment 136696 [details] Patch Cool

Comment on attachment 136696 [details] Patch Clearing flags on attachment: 136696 Committed r113918: <http://trac.webkit.org/changeset/113918>

All reviewed patches have been landed. Closing bug.