Issue metadata
Sign in to add a comment
|
63.5% regression in system_health.common_desktop at 530356:530497 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
Showing comments 7 - 106
of 106
Older ›
,
Jan 22 2018
re: #5 - wouldn't the presence of infobars affect the amount of space left for the content area, and therefore impact rendering? I agree it sounds like a bad test if that's the case. > This performance metric is incorrect. Not all tests are perfect. It's possible that your change did not regress performance, and only appears to be a problem because the test is measuring incorrectly. If this is the case, you must explain clearly what the issue with the test is, and why you believe your change is performance neutral. Please include data from traces or other performance tools to clarify your claim. +cc benchmark owners
,
Jan 22 2018
Hmm. I think this change could have made the infobar a couple pixels taller, but I would have assumed that would speed up page rendering rather than slowing it down.
,
Jan 22 2018
it could always cause new weirdness with, e.g., absolutely positioned elements that are now overlapping other elements. Or it could have changed the number of times we have to re-render everything (due to the infobar animation).
,
Jan 23 2018
pkasting, you can run the test locally to reproduce: From chromium/src/, run: ./tools/perf/run_benchmark system_health.common_desktop --browser=system --story-filter=load:search:google After the story run finished, you can also look at the trace it produced locally to investigate further.
,
Jan 23 2018
Sorry, the command should be: "./tools/perf/run_benchmark system_health.common_desktop --browser=release --story-filter=load:search:google" (if you built the "release" browser. Can also be replaced with "default")
,
Jan 24 2018
What I've found so far: * There's definitely an infobar showing -- the "unsupported flags" infobar, which appears due to --ignore-certificate-errors-spki-list. As a result, the height of the content area has changed, and in general, these perf tests will be affected by infobar changes. This seems bad. Do we have to pass this flag in? * Specifically, the height of the content area changed from 595 px to 589 px. * In the traces on the Linux perf bot, there's some kind of "animation" section under FrameLoader that has a bunch of entries that went from ~9 ms before the change to ~56 ms after. But I don't know what these are measuring or triggered by, and the trace viewer gives me little info on them. I think maybe this has something to do with the cc layer tree but the relevant classes and functions are basically completely undocumented (thanks cc/ folks). I don't really understand what I'm looking at enough to know how to proceed. I could use help from a renderer and/or cc person who understands how to interpret the traces around this region.
,
Jan 24 2018
It would probably be a good idea to run these tests without infobars, but just removing that flag may not be possible and even so wouldn't protect against future infobars cropping up. Perhaps the benchmark owners can tell us if there is a way to change the test to run in a popup window? I would be tempted to add a flag such as --suppress-infobars, but that would no doubt be abused (e.g. to hide the unsupported flags infobar).
,
Jan 24 2018
We had --disable-infobars for some other perf tests up to about a week ago, when I removed it because malware was abusing it.
,
Jan 24 2018
+Tim/Brian: can you help Peter with understanding the regression related to cc?
,
Jan 24 2018
,
Jan 24 2018
,
Jan 24 2018
nednguyen: Any chance we can stop passing --ignore-certificate-errors-spki-list? That would make the benchmark performance not vary based on changes to infobars, which would be nice.
,
Jan 24 2018
,
Jan 25 2018
--ignore-certificate-errors-spki-list is needed to make HTTPS connection works in our real world test cases.
,
Jan 25 2018
That's not enough detail for me to know whether there's a solution here. * Do we actually connect to real sites? If so it seems like this would introduce a lot of variance in the benchmark, and we should connect to something canned. * If we connect to something canned, can't we just connect over HTTP? * If somehow we need to use real sites, why do we actually need this flag? Seems like these sites shouldn't be throwing cert errors.
,
Jan 25 2018
#21: * We do need connect to real sites. Many teams in Chrome have been relying on these benchmarks to make performance work on real worlds site. For example, V8 team's analysis has shown that most synthetic benchmarks are not good at reflecting the real world profile: https://v8project.blogspot.com/2016/12/how-v8-measures-real-world-performance.html * Most of the most top websites (Google, FB, Amazon..) are now HTTPS only (for good reason) * Without using the flag, these site will throw cert error. In some sense, one can see using WPR to capture page snapshots & replay them as a "fishing attack". xunjeli@ is an expert on this & can give you more details.
,
Jan 25 2018
Re #22: I think there's a bit of miscommunication here. We don't connect to live sites, we use web page replay. I think what Peter is asking is if we could get WPR to serve sites which were recorded via https over http.
,
Jan 25 2018
#23: Ah. I see. I think that would make the tests a lot less realistic in term of performance. Helen would be able to give more details on this front as well
,
Jan 25 2018
Yes, I am asking what comment 23 asks. It would surprise me if tests that ostensibly test things like renderer performance are critically dependent on whether the network traffic is encrypted. That seems like a situation we don't want to be in (where unrelated factors are conflated in the measurements). And if the concern is realism, then clearly having an infobar about this is not realistic (users won't be using this flag) and massively affects the results (as we see from this bug), so we're already in a state where we're apparently very divorced from real-world performance. It would be nice to fix that :)
,
Jan 25 2018
#25: we used to have memory leak that are only captured if the tests are using HTTPS (go/chrome-net-memory-leak-postmortem). This test captures not only renderer performance but also how fast the page load, so exercising HTTPS stack is important. I can see thera are a few choices here are: 1) Serve requests through HTTP. --> Bad choice IMO. THis makes perf test less realistic, leading to us losing coverage in HTTPS 2) Make sure that we don't serve info-bar if --ignore-certificate-errors-spki-list flag is enabled. This may have some security consequence, so we need to check with security folks. 3) Accept this regression as this is not a real regression as real users won't see it. We still want to make sure that the noise level of the metrics do not increase, otherwise this test's capability to catch regression is reduced.
,
Jan 25 2018
We can't do (2), that's abusable by malware. (3) is a problem because we will still have this problem in the future. Comment 13 suggests running the test so the actual measurement occurs in a popup window. This or some similar solution (e.g. programmatically close the infobar before beginning timing and loading the site in question) represent a choice (4) I'm not seeing in your reply.
,
Jan 25 2018
#27: I am just suggesting all options I am seeing. Feel free to suggest more :-) I don't think (4) running the test so the actual measurement occurs in a popup window because that also make the test harder to write & unrealistic. We have around hundreds tests that involve HTTPS, some are complex user interactions such as "issue search queries on google.com home page, then scroll down to a result, click on it, interact with the next page...". I can't imagine rewritting all of them to be in a popup window. > (3) is a problem because we will still have this problem in the future. Can you explain further? Are you worrying about future changes to infobars will keep causing false regression? Are you expecting large amount of change to inforbar UI in the near future? If that's the case, can we guard the change in this areas through finch trial flag & flip them to reduce the number of possible false regressions?
,
Jan 26 2018
The idea of running tests in a popup window would not be to rewrite any of the tests, but simply rewrite the test launcher to launch the tests in such a window. But if the test launcher can do this, the test launcher can probably close the infobar before loading the test in question, and that would be better in pretty much every way. I don't know of a reason why that won't work. As for why (3) is a problem, yes, we'll be continuing to make changes to infobars. And even if we weren't, animating in the infobar has a meaningful effect on everything we do in the content area; its presence makes every test you're running nonrepresentative of real-world usage today. It seems like you should be concerned about that. I don't know what "guard through finch trial" means. Launching infobar changes through finch doesn't make them not affect these perf tests. We just add the extra overhead of a bunch of trial infrastructure for no reduction in problems, unless I'm misunderstanding the proposal.
,
Jan 26 2018
#29: does the infobar pop up as soon as the browser is started, or only when the HTTPS website is accessed? If it is the former, closing the infobar upon starting the browser could work for many cases. The exceptions are few cases which we test browser's startup loading (e.g: ./chrome google.com) or something like session restore. If it's the latter, that wouldnt work for the cases like "user launch http site, then click on a link on the page which navigates to HTTPS". Another issue is I am not sure how to tell the browser to close the infobar. If we have a browser API for doing that, isn't it essentially the same as adding a flag like --launch-browser-without-infobar? An option we can consider is adding a flag like --close-infobar-after-3s. We can then modify most of perf loading test to wait for 3 seconds before start the test. This may also mitigate malware abuse. > I don't know what "guard through finch trial" means. My proposal here is instead of making 30 commits which touch infobar & create 30 regressions, you would guard all of them through a finch trial flag. You will only need to do with 1 perf regression when you enable the flag flip. * Can you also explain to me the security model in the case of malware changing Chrome's startup flags? If we assume that malware have control over Chrome startup commandline on user machine, isn't all bets are off? What would prevent them from launching "bad_chrome.exe" instead of the actual Chrome? Another attack I can think of is they can launch "chrome.exe --start-fullscreen --app=https://my-evil-website-that-fakeout-browsingbar.com"
,
Jan 26 2018
I forget, does telemetry use official builds? If not, could we have the flag to disable the infobar only function in non-official builds?
,
Jan 26 2018
We can't get rid of --ignore-certificate-errors-spki-list and serve sites using http. Telemetry benchmarks use canned real-world webpages from top sites. Many of these webpages are originally served via https (e.g. google search in this case). We can't modify all https links to http. Maybe it's feasible to iterate through all links in the archive and change them to http, but things might break and we would definitely lose a lot of code coverage. To be as close as to the real world as possible (replaying https sites using https and http sites using http), we need to make Chrome trust WPRGo which is essentially a MITM proxy. --ignore-certificate-errors-spki-list is the only testing flag that allows us to do so.
,
Jan 26 2018
Or a local trust root installed on the test machine with WPRGo's cert. Wouldn't that also work?
,
Jan 26 2018
+Patrick: we went through the pat of installing local trust root once. The challenges we faced were: 1) We can't figure out how to make it work across all platforms. We was able to make it work on Android K, Clank folks was about to make it work on Linux and that's pretty much it. 2) Even if it works, it's very fragile. Our Android solution was broken on Android M+. 3) It requires us to have root access on swarming bots, which is against the recommended practice of tests should have minimal privilege on testing environment.
,
Jan 26 2018
,
Jan 26 2018
I think this infobar only shows up on the first browser window, so you could modify the test harness to run the test in a second window (doesn't have to be a popup window). But there are other infobars out there and it would be nice if in the future when people are working on browser UI it doesn't affect tests like this. If we're in control of the build step we could add a compile-time flag (goes back to #31).
,
Jan 26 2018
#36: as mentioned, I think solutions in the realm of modifying test harness to avoid the popup window is essentially the same as adding "--suppress-info-bar-after-it-pops-up" flag. I am fine with this solution, except that it still not work for case of testing browser startup performance with preloaded HTTPS content. I think adding a compile-flag is a solution worth considering as well.
,
Jan 26 2018
The infobar appears on browser start, not on page load, and it appears in the first tab only, so creating a new tab and closing the first one would work. If the perf tests build Chrome themselves and are willing to pass custom #defines as part of that build process, I would be happy to add a #define to the infobar code to simply disable infobars. That's much less abusable than a command-line flag :)
,
Jan 29 2018
FWIW, when telemetry launches chrome, it does the initial set-up over devtools (gets system/gpu info, etc.). As part of that, telemetry could ask chrome to close the infobar. I think that would be a better solution than having a separate binary (once there's a separate binary, it gets much easier for that to diverge from the main binary that we want to test).
,
Jan 29 2018
If we don't already have a separate binary, then I agree that trying to manually close the infobar would be better. This can be a bit problematic, though, as the infobar animates while opening and closing, and we'd likely want to wait for the animation to finish before starting any loads to ensure there's no perf impact. We could eliminate that, if need be, by avoiding infobar animations when the rest of Chrome has "rich UI animations" disabled; that in turn is controllable by the host OS, I believe. (There may also be a command-line flag, I'm not sure.)
,
Jan 29 2018
I can see there are two problems with adding an extra APIs to close infobar: 1) It will still affect performance of browser startup tests. For these tests, we measure the time from browser process started to when things got display (first pain/first contentful paint) 2) It is basically a more cumbersome way of implement "--suppress-inforbar-popup". If Telemetry can run some magical comment to stop infobar, malware should be able to the same thing. This is just security by obscurity.
,
Jan 29 2018
It's not magical; Telemetry would be triggering the infobar close button via clicking it. We're not proposing adding APIs.
,
Jan 29 2018
#42: Telemetry have been interacting with the browser through devtool APIS and not through any platform APIs. Eeverything we used is documented in https://chromedevtools.github.io/devtools-protocol/ How do you propose we implement the action of "clicking infobar close button"? I imagine that this infobar is not part of any renderer content?
,
Jan 29 2018
It's part of the browser UI and it's a focusable element. If you can send keypresses, you can click the button (by changing focus to it and then sending a spacebar press).
,
Jan 29 2018
sadrul@/Pavel: do you know if devtool already expose APIs to interact with browser UI components?
,
Jan 29 2018
The gpu benchmarking extension would presumably enable this, via https://cs.chromium.org/chromium/src/third_party/catapult/telemetry/telemetry/internal/actions/key_event.py?q=keypressaction&dr=CSs .
,
Jan 29 2018
Actually, I think key presses are targeted directly to the renderer. You'd need to use a form of input which goes through the whole stack, like touch.
,
Jan 29 2018
My other concern remains the same: if Telemetry can automate closing the pop up, what prevents the malware to do the same? If time it takes for the pop up to disappear is the only difference that makes it more secure, why not just implement a flag like "--stop-pop-up-after-3-seconds"? :-)
,
Jan 29 2018
Malware that automates not only launching Chrome but sending keypresses to it can hose the user far more than this. Also, I'm less concerned about programmatically closing this specific infobar than programatically disabling all infobars -- various other infobars (which don't have close buttons) are more problematic. That said, I don't want to make it trivial to close this infobar either. Having to focus and interact with elements in the browser is a reasonably high bar to cross because it requires an active process sending events on the local machine. Just setting a command-line flag on the user's Chrome shortcut is easier to do in practice. The time it takes the popup to close is not a relevant factor in any security considerations.
,
Jan 29 2018
Are you saying that malware can easily use the devtools APIs? I would have thought there's lots of stuff there that we wouldn't want available to malware. We have private extensions apis like autotest_private which are presumably off-limits to anything except specially whitelisted testing extensions.
,
Jan 29 2018
#49: so we are following the model security-by-obscurity. I wonder whether it's easier & more secure to use some sort of private key to disable the infobar. We can make sure that only our bots & Googlers can have access to the keys.
,
Jan 29 2018
@51: No, not "obscurity", "levels of privilege". I don't know whether this is your intent, but I'm beginning to feel a bit like the focus is on attacking elements of the surrounding system (how we show messages, what security practices are in place, etc.) as being poorly-motivated rather than on solving the immediate problem. Here are the solutions I've heard proposed so far, and their downsides: (1) Play back pages over HTTP. Would require rewriting page content and would fail to exercise portions of the stack that we care about exercising. Non-starter. (2) Install a local trust root. Fragile, and might not be possible on all platforms. Also requires root access on bots. If we thought we could make this work everywhere it might be worth doing anyway, but since we don't, this is likely a non-starter. (3) Build a separate binary that uses a #define to disable infobars. Fixes all perf test issues, but requires maintaining a separate binary (which might diverge over time), and might in theory be problematic for other, non-perf uses of the same binary (e.g. bisections). (4) Re-add a command-line flag to disable infobars entirely. Previously removed due to known abuse in the wild by malware. Non-starter. (5) Add a command-line flag to selectively disable infobars of certain types/disable this infobar using a private key/something. Might work, adds more code complexity and key management issues. (6) Have the test suite manually close the infobar. Requires telemetry to be able to send input events to the browser process, which maybe it can't do? Would still impact startup timing unless we disable infobar animations (and maybe even then), which adds complexity in multiple areas. (7) Have the test suite open a new tab first, then run the tests in that. Might be easier to implement than closing the infobar, would still affect startup timing (in different ways than the previous solution). (8) Do nothing. Means perf tests aren't as representative of real usage and are subject to future false changes (positive and negative) as infobars change. My preference is probably for (3), then (8), then (6)/(7). If we do (8), I'd still like to first understand why this change caused such a meaningful regression in the numbers (see comments 12/15). Changing the infobar height by a couple of pixels led to a 63.5% regression? This seems wildly off, and I don't think we should close without knowing what's up there.
,
Jan 29 2018
#52: I am sorry that you feel this way, but I have proposed many solutions & explained the challenges we faced each of them in thoroughly. So far none of the proposed solution satisfies all of these criteria: easy to implement, secured, do not diverge the binary we test from what we ship to users, completely eliminate unintended perf effects of infobar on all test cases. My intent to look at this from security practices is to better understand the concern & explore further approaches to tackle this. I have no other motivation besides finding solutions that maximizes utilities on all mentioned criteria. This is a hard problem, so I hope we all keep our best intent when searching for the good solutions. If you want to time-bound this bug, I suggest we do (8), and understand the regression as you suggested. Meanwhile, we can put the problem of infobar under perf test in our backlog & keep brainstorming other ideas.
,
Jan 29 2018
We've seen this issue outside of infobar space as well. Peter's (3) and Tim's #31 are fairly close and I think introducing a subset of command line flags that would only be available in non-official builds could solve the issue without adding much into the maintenance of the separate binary.
,
Jan 29 2018
I missed that solution. Indeed, if telemetry builds are non-official, then I'm happy to add some sort of --disable-infobars that only works in non-official builds and have telemetry use that.
,
Jan 29 2018
Ned tells me telemetry builds are currently official. So if we want a solution like the above, we could either: (a) Stop building with -DOFFICIAL_BUILD, then add a --disable-infobars that only works in non-official builds, and pass that (b) Do (3) directly, i.e. start building with some new -DDISABLE_INFOBARS Either way diverges from what we ship to users. (b) is maybe easier to implement. (a)'s other effects are unclear to me because I don't know what OFFICIAL_BUILD controls. If it affects anything about optimizations, then that route would be out. +CC brucedawson as a guess at who might know what disabling OFFICIAL_BUILD would do. On an unrelated note, Ned also tells me telemetry can't send input events at all, so (6) isn't doable.
,
Jan 29 2018
I was going to suggest having the flag (to hide infobars), but ignoring it on STABLE channels. But I think telemetry also runs on stable channels? So that probably won't work. As estade@ mentioned above: if malware is able to setup a devtools connection to the browser, then it can do a lot more damage. So having the ability in devtools (with an browser/infobar-specific devtools API) to close the inforbar should at least be no worse than how things are right now, in terms of security. Because this is a specific API implemented inside chrome, it will have all the information about ongoing animation in the infobar etc., so it should be able to correctly close the infobar, and not have to worry about the state of the browser animation etc.
,
Jan 29 2018
We currently have a separate "your browser is being remotely debugged" sort of infobar for dev tools, but maybe this is a different kind of "dev tools control" than that? I don't fully understand all the different ways that we can partially-remote-automate Chrome today.
,
Jan 30 2018
Adding the devtools API does mean that we'd be testing the same binary we're shipping, but we're still testing code paths that aren't a part of a normal Chrome run. Why is including dead code in production superior to compiling it out of production?
,
Jan 30 2018
Right now the only way to control whether -DOFFICIAL_BUILD is set is to set/not-set the gn arg "is_official_build". So, in addition to the ~130 places where we check defined(OFFICIAL_BUILD) in .cc and .h files we also change some optimization settings, at least in VC++ builds (is_official_build implies Link Time Code Generation/WPO). The LTCG/WPO difference is going away as clang takes over, but it might come back, and it sometimes makes a non-trivial difference. I think I'd be more concerned about all of the places that check defined(OFFICIAL_BUILD) - I don't think it's possible to understand all of the ways in which that changes behavior and performance.
,
Jan 30 2018
> We currently have a separate "your browser is being remotely debugged" sort of infobar for dev tools, but maybe this is a different kind of "dev tools control" than that? This infobar is shown when certain extensions api is used. Running chrome with --remote-debugging-port does not show it.
,
Jan 30 2018
re #59: my only worry is that having a separate binary makes it much easier to add more things into said binary, and it would diverge from the main binary in a way that would make a difference to what the perf numbers actually mean. .. although, stepping back a bit: maybe once we understand how the current regression is happening, we can resolve this better? For example, instead of trying to hide the infobar, we can make sure the browser-size is adjusted so that the web-page window remains as the test expects it to be?
,
Jan 30 2018
#62: I think that can be mitigated with the code review process. While I agree that your concern is valid, I think of it as a call for design pattern so that we can effectively track of what are differences between what we test & what we ship instead. Given Bruce's assessment in #60, I favor adding -DDISABLE_INFOBARS flag to perf build instead of switching off -DOFFICIAL_BUILD
,
Jan 30 2018
We have slowly started to look at measuring performance of the chrome UI. I guess we will have to leave infobars out of it, then (although we weren't planning on looking at infobar immediately ... but maybe later in the year).
,
Jan 30 2018
"Ned also tells me telemetry can't send input events at all, so (6) isn't doable." We pass --enable-gpu-benchmarking for our telemetry tests, which enables synthesizing input at the browser level, for some forms of input. I believe we could use this API to close infobars.
,
Jan 30 2018
#65, Tim: it's still unclear how do we issue gesture to focus on the infobar element. Also startup tests will still be affected by the infobar's performance, so I still favor option (3) (add extra build flag to disable infobar) over (6)
,
Jan 30 2018
It would be a bit painful. Either we hardcode the location of the button to close the infobar, or we add an API to get it's location, I suppose. It's reasonably likely that other options are better, but I suspect this is theoretically possible.
,
Jan 30 2018
I don't think we should extend --enable-gpu-benchmarking, we've been trying to walk away from it for quite some time. It also does not seem to be distinguishable from restoring the --disable-infobars from the malware standpoint, does it?
,
Jan 31 2018
Re #68, we aren't going to move away from exposing browser level input synthesis though, we're just planning to move it out of GPU benchmarking. I agree that adding an API to get the infobar location is pretty messy though, and lands us in a similar situation as restoring --disable-infobars.
,
Jan 31 2018
OK, from what I'm reading so far, it seems like the leading candidate solution is to add a -DDISABLE_INFOBARS and have perf builds set this. If comment 64 is a concern, we could restrict this to something very targeted, e.g. -DDISABLE_BAD_FLAGS_INFOBAR, which would allow infobars to appear for every other reason. I don't know if that's preferable. I can take care of adding the flag and making it work if someone else can take care of making the perf builds actually use this. Volunteers?
,
Feb 5 2018
Another possible direction: When one or more flags is set (--enable-automation/--remote-debugging-port?), disable all infobars, and instead overlay the whole top chrome with some kind of 50% opaque white layer, in the middle of which is "Browser automation is enabled _Learn more_ X". The link would go to a page explaining to end users what this means and what to do if they are in this state (presumably, look for malware?). The X closes the overlay, so humans debugging web driver tests can still get at the browser controls. This requires more engineering work on the UX side (in particular, at least views and Mac impls and localization, along with help center content), but avoids a separate binary, which would be nice for the testing pipeline.
,
Feb 5 2018
I like #71 a lot.
,
Feb 5 2018
Having parts of the browser ui overlayed on top of the web-page can potentially affect the performance numbers (overdraw etc.)
,
Feb 5 2018
@73: Not on the web page, on the top chrome.
,
Feb 5 2018
#71 probably would also address the security problem with Chrome devtool being opened, so I like it as well. One problem I see is is malware may open chrome with --start-fullscreen which would hide all the Chrome UI. So maybe if Chrome is in fullscreen momde, we make an infobar ""Browser automation is enabled _Learn more_ X"?
,
Feb 5 2018
,
Feb 5 2018
I would address that by making this kick on only when --start-fullscreen is not passed. When it _is_ passed, we follow the normal codepath, i.e. we show any infobars we normally would (which will now appear at the top of the screen).
,
Feb 5 2018
#77 SGTM
,
Feb 6 2018
+1 to #71 & #77.
,
Feb 6 2018
We will still be doing a bit more work with #71 (i.e. there will still be overdraw), but hopefully not enough to impact the perf data.
,
Feb 6 2018
,
Feb 7 2018
Btw: do we understand why the small change in size causes this amount of regression?
,
Feb 7 2018
@82: No; see comments 12/15/52. There's been radio silence on help figuring this out, which worries me.
,
Feb 7 2018
+ Blink>Compositing Label. Resurfacing #12. Brian, any thoughts on who could help dig in here? " * There's definitely an infobar showing -- the "unsupported flags" infobar, which appears due to --ignore-certificate-errors-spki-list. As a result, the height of the content area has changed, and in general, these perf tests will be affected by infobar changes. This seems bad. Do we have to pass this flag in? * Specifically, the height of the content area changed from 595 px to 589 px. * In the traces on the Linux perf bot, there's some kind of "animation" section under FrameLoader that has a bunch of entries that went from ~9 ms before the change to ~56 ms after. But I don't know what these are measuring or triggered by, and the trace viewer gives me little info on them. I think maybe this has something to do with the cc layer tree but the relevant classes and functions are basically completely undocumented (thanks cc/ folks). I don't really understand what I'm looking at enough to know how to proceed. I could use help from a renderer and/or cc person who understands how to interpret the traces around this region. "
,
Feb 7 2018
We have to mark this as Untriaged for Internals>Compositing to see it for triage. Also, anyhting in cc/ is Internals>Compositing, not Blink>Compositing. Although this may turn out to be Blink>Compositing.
,
Feb 8 2018
Re comment #84: The "animation" section under FrameLoader is likely longer because of the extra time the main thread is blocking on the compositor thread, which is blocking for a really long time (42ms) on what looks like the GPU service. The GPU service does look like it's doing significantly more work after the regression. +sunnyps: Do you know why the compositor might be blocking on the GPU service in this case?
,
Feb 12 2018
,
Mar 8 2018
@pmeenan, nednguyen: who drives those tests? do we pass --enable-automation there? It disables infobar animations and should bring perf numbers back to reasonable.
,
Mar 8 2018
We don't pass --enable-automation in Telemetry yet. It should be simple to add. +Juan: since I am gone this week. Feel free to pick up passing "--enable-automation" in Telemetry browser's startup args
,
Mar 8 2018
CL to add the flag: https://chromium-review.googlesource.com/c/catapult/+/955586
,
Mar 10 2018
I don't know that I would suggest --enable-automation when tests don't need it. I'd prefer some sort of --disable-infobar-animation flag, or to infer that from somewhere. --enable-automation will cause a second infobar to be shown currently, which seems problematic.
,
Mar 10 2018
I'd see what it does to the perf numbers and go from there?
,
Mar 12 2018
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/17eb2426440000
,
Mar 12 2018
📍 Couldn't reproduce a difference. https://pinpoint-dot-chromeperf.appspot.com/job/17eb2426440000
,
Mar 12 2018
Results from pinpoint job to compare with/without the flag should be here: https://pinpoint-dot-chromeperf.appspot.com/results2/17eb2426440000 But I'm getting a "403 Forbidden" error trying to access them. +simonhatch can you help us with that?
,
Mar 12 2018
Give it a try now.
,
Mar 14 2018
Hope others can access the results link in #95 now. If you select "without" patch as the reference, there are some improvements and some regressions, but the :netural_face: next to each change says that the changes are not significant. So, what do people think? Should the flag be added? Do we need a better flag?
,
Mar 14 2018
(significant -> statistically significant)
,
Mar 26 2018
Peter: do you still need to investigate the actual reason behind why infobar's UI change cause the regression on loading? I think this bug is a bit hard to follow because it contains many different problems: 1) Why infobar UI change cause regression on loading? 2) How to stop infobar UI on Telemetry perf tests. (issue 825872) 3) Current security problem with devtool. (issue 825873) I split (2) & (3) to separate bugs to make this bug focused on the performance problem with the infor bar UI change.
,
Mar 26 2018
I would like to understand (1), yes, but I don't consider it within my power to investigate; I'm waiting for comment from sunnyps in re: #86. I think you combined a couple different issues on bug 825872. Building an alternate UI for GlobalConfirmInfoBarDelegate should be a separate bug compared to if Telemetry wants --enable-automation (which I'm not convinced it does, but maybe). That separate bug is really the only thing that should be assigned to me. I don't know what bug 825873 is about.
,
Mar 28 2018
Morphing. This bug is now only about understanding why such a small infobar change triggered such a large perf effect. This seems bad. Other followup work, e.g. changing the UI here to not use infobars, is tracked in bug 818483 and various other bugs. Reassigning based on comment 86.
,
Apr 6 2018
,
Jul 19
,
Jul 20
,
Jul 30
,
Aug 2
Showing comments 7 - 106
of 106
Older ›
|
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||