Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10916292:
Adaptively throttle texture uploads (Closed)
|
Created:
8 years, 3 months ago by brianderson Modified:
8 years, 3 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdaptively throttle texture uploads
We need adaptive texture uploading because we run on a wide variety
of systems and a one size fits all policy will not work.
This patch maintains a tick rate of 4ms and default upload count of
12 textures per tick, but allows for more textures per tick if we've
measured that we can be faster.
The number of partial updates allowed is also communicated from the
impl thread to the main thread at the beggining of each frame.
BUG=145825
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157473
Patch Set 1 #
Total comments: 5
Patch Set 2 : Add tests. #
Total comments: 14
Patch Set 3 : Address comments in PS2, except constant partials #
Total comments: 12
Patch Set 4 : Address comments #Patch Set 5 : rebase #Patch Set 6 : cc_unittests passing again #
Total comments: 2
Patch Set 7 : Fix CCTextureUpdateController::updateMoreTexturesNow due to rebase #Patch Set 8 : Do not vary partial upload limit #
Total comments: 15
Patch Set 9 : Rebase. Estimate in textures not pixels. #
Total comments: 6
Patch Set 10 : rename maxPartialTextures #Patch Set 11 : 1lu #Patch Set 12 : Fix platform specific compile errors. #
Messages
Total messages: 35 (0 generated)
Continuing review from https://bugs.webkit.org/show_bug.cgi?id=96067. This patch synchronizes the number of texture uploads on the main and impl threads as each thread takes it's turn at modifying the final value. One outstanding concern is whether we should be modifying the number of texture uploads or the tick rate.' Also, correctness and performance tests are still to come.
https://codereview.chromium.org/10916292/diff/1/cc/CCLayerTreeHost.cpp File cc/CCLayerTreeHost.cpp (right): https://codereview.chromium.org/10916292/diff/1/cc/CCLayerTreeHost.cpp#newcod... cc/CCLayerTreeHost.cpp:510: *maxTextureUpdates = m_maxPartialTextureUpdates; // We need to communicate that we've clamped. Actually, I shouldn't communicate that we've clamped here. Otherwise partial texture upload limits on the main thread will affect the full texture upload limit on the impl thread. https://codereview.chromium.org/10916292/diff/1/cc/CCThreadProxy.cpp File cc/CCThreadProxy.cpp (right): https://codereview.chromium.org/10916292/diff/1/cc/CCThreadProxy.cpp#newcode64 cc/CCThreadProxy.cpp:64: , m_mostRecentMaxTextureUpdatesOnMainThread(12) Will fix this hard coded constant. https://codereview.chromium.org/10916292/diff/1/cc/CCThreadProxy.cpp#newcode461 cc/CCThreadProxy.cpp:461: size_t maxTextureUpdates = 12; Will also fix this hard-coded constant. https://codereview.chromium.org/10916292/diff/1/cc/CCThreadProxy.cpp#newcode479 cc/CCThreadProxy.cpp:479: Need to store the most recent maxTextureUpdates on the main thread for compositeAndReadback.
why on earth? https://codereview.chromium.org/10916292/diff/1/cc/CCLayerTreeHost.h File cc/CCLayerTreeHost.h (right): https://codereview.chromium.org/10916292/diff/1/cc/CCLayerTreeHost.h#newcode235 cc/CCLayerTreeHost.h:235: void updateLayers(LayerChromium*, CCTextureUpdateQueue&, size_t *maxTextureUpdates); wat?
> why on earth? Yeah, I wasn't thinking straight last night. Changing it as we speak.
Hehe sorry :)
This adds a metric to the texture upload benchmark to measure average commit time. It works, but I need to test it perf with and without this patch more thoroughly. On a Z620, this patch improves some cases and regresses others, but the data could be noisy: https://docs.google.com/a/google.com/spreadsheet/ccc?key=0AsLfT0pODDuEdExteDV...
Is it easy to separate out the stats changes from the throttling changes?
https://codereview.chromium.org/10916292/diff/8001/cc/CCTextureUpdateControll... File cc/CCTextureUpdateController.cpp (left): https://codereview.chromium.org/10916292/diff/8001/cc/CCTextureUpdateControll... cc/CCTextureUpdateController.cpp:69: uploader->uploadTexture(resourceProvider, queue->takeFirstPartialUpload()); I think we should do partials all at once, and swamp the GPU if we want. And, we should just set max partials conservatively. To like, 8. Its my suspicion that partials come very infrequently, in twos or threes, not huge vast numbers... and, though I might be wrong, I'd rather we do this just for the non partials case first, then come back to the other when we've got data in hand. Wdyt? https://codereview.chromium.org/10916292/diff/8001/cc/CCTextureUpdateControll... File cc/CCTextureUpdateController.cpp (right): https://codereview.chromium.org/10916292/diff/8001/cc/CCTextureUpdateControll... cc/CCTextureUpdateController.cpp:86: // Flush periodically, but deffer our last flush until after uploader->endUploads() deffer->defer https://codereview.chromium.org/10916292/diff/8001/cc/ThrottledTextureUploade... File cc/ThrottledTextureUploader.cpp (right): https://codereview.chromium.org/10916292/diff/8001/cc/ThrottledTextureUploade... cc/ThrottledTextureUploader.cpp:18: static const size_t uploadHistorySize = 10; This is in number of queries, right? I might make this be bigger, like 50 or 100, or even longer. The whole system can destabilize during big transitions, or when you've opened a background page... so having a big history is maybe helpful? https://codereview.chromium.org/10916292/diff/8001/cc/ThrottledTextureUploade... cc/ThrottledTextureUploader.cpp:20: // Default number of pixels per second. This seems a bit low, no? If you have 1.5ms per texture as it seems to be on CrOS now, then thats like 666 textures per second. Each texture is 65536 pixels in this metric. So um... math fails me. https://codereview.chromium.org/10916292/diff/8001/cc/ThrottledTextureUploade... cc/ThrottledTextureUploader.cpp:65: { can you reverse this so that you have m_hasValue, m_value so value() { if (m_hasValue) return m_value; wait; return m_value; } requires more temporaries but is I think cleaner flowing. https://codereview.chromium.org/10916292/diff/8001/cc/ThrottledTextureUploade... cc/ThrottledTextureUploader.cpp:112: if (m_pixelsPerSecondHistory.empty()) Instead of this, seed the queue with max entries worth of the default value. That gives us a gradual transition to a better guess. Also, I think we should try putting a global int that is mutable that is our last guess. And, then, have that be the initial guess as well. So when you navigate from page to page, we start off with the old guess. https://codereview.chromium.org/10916292/diff/8001/cc/ThrottledTextureUploade... cc/ThrottledTextureUploader.cpp:116: std::deque<double> sortedHistory(m_pixelsPerSecondHistory); why deque and not a vector?
https://codereview.chromium.org/10916292/diff/8001/cc/CCTextureUpdateControll... File cc/CCTextureUpdateController.cpp (left): https://codereview.chromium.org/10916292/diff/8001/cc/CCTextureUpdateControll... cc/CCTextureUpdateController.cpp:69: uploader->uploadTexture(resourceProvider, queue->takeFirstPartialUpload()); Do you mean we should not upload fulls and partials in the same tick? https://codereview.chromium.org/10916292/diff/8001/cc/CCTextureUpdateControll... File cc/CCTextureUpdateController.cpp (right): https://codereview.chromium.org/10916292/diff/8001/cc/CCTextureUpdateControll... cc/CCTextureUpdateController.cpp:86: // Flush periodically, but deffer our last flush until after uploader->endUploads() ok https://codereview.chromium.org/10916292/diff/8001/cc/ThrottledTextureUploade... File cc/ThrottledTextureUploader.cpp (right): https://codereview.chromium.org/10916292/diff/8001/cc/ThrottledTextureUploade... cc/ThrottledTextureUploader.cpp:18: static const size_t uploadHistorySize = 10; I intentionally kept it low so that we would quickly adapt to any short-term bumps. The thinking being that we'd want to back off on texture uploads under high load to keep animations smooth. https://codereview.chromium.org/10916292/diff/8001/cc/ThrottledTextureUploade... cc/ThrottledTextureUploader.cpp:20: // Default number of pixels per second. It is low. What I've calculated below is actually pixels per 16ms. (48 came from 12 textures per 4ms.) Will fix. https://codereview.chromium.org/10916292/diff/8001/cc/ThrottledTextureUploade... cc/ThrottledTextureUploader.cpp:65: { Ok. Avoiding the call to getQueryObjectuivEXT multiple times is probably a good idea. https://codereview.chromium.org/10916292/diff/8001/cc/ThrottledTextureUploade... cc/ThrottledTextureUploader.cpp:112: if (m_pixelsPerSecondHistory.empty()) Sounds good. https://codereview.chromium.org/10916292/diff/8001/cc/ThrottledTextureUploade... cc/ThrottledTextureUploader.cpp:116: std::deque<double> sortedHistory(m_pixelsPerSecondHistory); My thinking was that the deque copy constructor would be faster than the vector iterator constructor and that it could offset the additional CPU overhead (which may potentially be hidden anyway) of indexing the deque during sort. I have not done any measurements to see if that's true but, now that I think about it, deque indexing will have conditions that can break the pipeline more than vector indexing. Probably safer to go with vector.
I removed the tests and will put them in a separate patch. Also addressed most of the comments from patch set 2. I haven't changed partial texture uploads back to a constant though. I can get to that on Monday if we decide to go that route. Doing so will simplify this patch, but I'd rather not undo the work only to have to redo it later on if, for example, we need it on mobile.
https://codereview.chromium.org/10916292/diff/1016/cc/CCTextureUpdateControll... File cc/CCTextureUpdateController.cpp (right): https://codereview.chromium.org/10916292/diff/1016/cc/CCTextureUpdateControll... cc/CCTextureUpdateController.cpp:64: uploadMore = queue->fullUploadSize() && fullUploadCount < count; hm, I feel like this has grown into something that looks complicated when it's not. I think the need for a comment to explain what we're doing here is a warning sign. Maybe something like this would have a cleaner flow? size_t fullUploadCount = 1; uploader->uploadTexture(...); while (fullUploadCount < count) { if (!queue->fullUploadSize()) break; if (!(fullUploadCount % textureUploadFlushPeriod)) resourceProvider->shallowFlushIfSupported(); uploader->uploadTexture(...); fullUploadCount++; } resourceProvider->shallowFlushIfSupported(); https://codereview.chromium.org/10916292/diff/1016/cc/CCTextureUpdateControll... File cc/CCTextureUpdateController.h (right): https://codereview.chromium.org/10916292/diff/1016/cc/CCTextureUpdateControll... cc/CCTextureUpdateController.h:24: static size_t maxTextureUpdates(TextureUploader*); why are we removing partial from the name? maxTextureUpdates doesn't seem descriptive enough to me. Maybe it should be maxTextureUpdatesPerTick? https://codereview.chromium.org/10916292/diff/1016/cc/CCTextureUpdateControll... cc/CCTextureUpdateController.h:28: nit: unnecessary newline. https://codereview.chromium.org/10916292/diff/1016/cc/CCThreadProxy.cpp File cc/CCThreadProxy.cpp (right): https://codereview.chromium.org/10916292/diff/1016/cc/CCThreadProxy.cpp#newco... cc/CCThreadProxy.cpp:97: beginFrame(m_mostRecentMaxTextureUpdatesOnMainThread); I think it's fine to not use partial updates at all for compositeAndReadback. How about we just do beginFrame(0) here and you can get rid of the m_mostRecentMaxTextureUpdatesOnMainThread variable completely? https://codereview.chromium.org/10916292/diff/1016/cc/CCThreadProxy.h File cc/CCThreadProxy.h (right): https://codereview.chromium.org/10916292/diff/1016/cc/CCThreadProxy.h#newcode94 cc/CCThreadProxy.h:94: void beginFrame(size_t maxTextureUpdates); should this be maxPartialTextureUpdates? https://codereview.chromium.org/10916292/diff/1016/cc/ThrottledTextureUploader.h File cc/ThrottledTextureUploader.h (right): https://codereview.chromium.org/10916292/diff/1016/cc/ThrottledTextureUploade... cc/ThrottledTextureUploader.h:58: bool m_hasValue; nit: unnecessary whitespaces between bool and m_hasValue.
https://codereview.chromium.org/10916292/diff/1016/cc/CCTextureUpdateControll... File cc/CCTextureUpdateController.cpp (right): https://codereview.chromium.org/10916292/diff/1016/cc/CCTextureUpdateControll... cc/CCTextureUpdateController.cpp:64: uploadMore = queue->fullUploadSize() && fullUploadCount < count; Sounds good. I was able to make it shorter and more straightforward using a similar structure. https://codereview.chromium.org/10916292/diff/1016/cc/CCTextureUpdateControll... File cc/CCTextureUpdateController.h (right): https://codereview.chromium.org/10916292/diff/1016/cc/CCTextureUpdateControll... cc/CCTextureUpdateController.h:24: static size_t maxTextureUpdates(TextureUploader*); Renaming back to partial texture updates. Will use a separate function for normal texture updates that, for now, will have the same implementation. https://codereview.chromium.org/10916292/diff/1016/cc/CCTextureUpdateControll... cc/CCTextureUpdateController.h:28: On 2012/09/15 17:26:19, David Reveman wrote: > nit: unnecessary newline. Done. https://codereview.chromium.org/10916292/diff/1016/cc/CCThreadProxy.cpp File cc/CCThreadProxy.cpp (right): https://codereview.chromium.org/10916292/diff/1016/cc/CCThreadProxy.cpp#newco... cc/CCThreadProxy.cpp:97: beginFrame(m_mostRecentMaxTextureUpdatesOnMainThread); Sounds good. https://codereview.chromium.org/10916292/diff/1016/cc/CCThreadProxy.h File cc/CCThreadProxy.h (right): https://codereview.chromium.org/10916292/diff/1016/cc/CCThreadProxy.h#newcode94 cc/CCThreadProxy.h:94: void beginFrame(size_t maxTextureUpdates); On 2012/09/15 17:26:19, David Reveman wrote: > should this be maxPartialTextureUpdates? Done. https://codereview.chromium.org/10916292/diff/1016/cc/ThrottledTextureUploader.h File cc/ThrottledTextureUploader.h (right): https://codereview.chromium.org/10916292/diff/1016/cc/ThrottledTextureUploade... cc/ThrottledTextureUploader.h:58: bool m_hasValue; On 2012/09/15 17:26:19, David Reveman wrote: > nit: unnecessary whitespaces between bool and m_hasValue. Done.
https://codereview.chromium.org/10916292/diff/12015/cc/CCTextureUpdateControl... File cc/CCTextureUpdateController.cpp (right): https://codereview.chromium.org/10916292/diff/12015/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:54: void CCTextureUpdateController::updateTextures(CCResourceProvider* resourceProvider, TextureCopier* copier, TextureUploader* uploader, CCTextureUpdateQueue* queue, size_t count) The previous patch set had a few bugs that caused back to back flushes, so I needed to reorganize the logic again. https://codereview.chromium.org/10916292/diff/12015/cc/CCTextureUpdateControl... File cc/CCTextureUpdateControllerTest.cpp (left): https://codereview.chromium.org/10916292/diff/12015/cc/CCTextureUpdateControl... cc/CCTextureUpdateControllerTest.cpp:140: onEndUploads is called before the final flush to keep it grouped with the texture uploads, so this check can no longer be done here. I move it to onBeginUploads and at the end of each test.
Latest patch doesn't vary the partial upload limit and pegs the partial upload limit at 12 (unchanged). reveman also indicated he would like to only measure full uploads in the bandwidth calculations. I won't get to that tonight though. Would it be okay to do that as a separate patch? It will make this patch bigger as opposed to smaller.
LGTM https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... File cc/CCTextureUpdateController.cpp (right): https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:21: static const size_t textureUpdatesPerFullTickMin = 12; i think this goes as low as 3 on android... thoughts?
I think this needs a bit more work. I created this https://codereview.chromium.org/10937007/ to removed throttling of partial uploads. I'd prefer if we landed that first as I think it would make this patch much cleaner but I could be convinced that it's ok to do this clean up in a follow up CL instead. https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... File cc/CCTextureUpdateController.cpp (right): https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:18: static const size_t pixelsPerTexture = 256 * 256; We could get rid of this awkward constant if partial updates were not considered. https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:21: static const size_t textureUpdatesPerFullTickMin = 12; We should be able to just remove this now that partial updates are always done in finalize(). maxFullUpdatesPerTick could just use 1 instead. https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:46: (size_t) floor(textureUpdateTickRate * texturesPerSecond)); Use static_cast here instead of c-style cast. https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:51: TRACE_EVENT0("cc", "CCTextureUpdateController::updateTextures"); You wouldn't have to touch this function at all if we stopped measuring partial updates. https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:67: bool cantUpdloadAllPartials = (count - fullUploadCount) < queue->partialUploadSize(); nit: spelling https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:68: if (cantUpdloadAllPartials) { here too https://codereview.chromium.org/10916292/diff/24001/cc/ThrottledTextureUpload... File cc/ThrottledTextureUploader.cpp (right): https://codereview.chromium.org/10916292/diff/24001/cc/ThrottledTextureUpload... cc/ThrottledTextureUploader.cpp:100: } nit: prefer if you keep the blank line you removed here https://codereview.chromium.org/10916292/diff/24001/cc/test/CCTiledLayerTestC... File cc/test/CCTiledLayerTestCommon.h (right): https://codereview.chromium.org/10916292/diff/24001/cc/test/CCTiledLayerTestC... cc/test/CCTiledLayerTestCommon.h:139: virtual double estimatedPixelsPerSecond() OVERRIDE { std::numeric_limits<double>::max(); }; nit: unnecessary semicolon
Why do you think this needs more work?
On 2012/09/18 06:12:48, nduca wrote: > Why do you think this needs more work? I bothers me a bit that it includes a lot of changes that would be unnecessary if we just didn't throttle partial uploads but I can live with that if we want to get this in before the branch point. Patch looks really good otherwise. Just like to see the other minor issues I pointed out in my last review fixed before landing.
I'm fine if you can land your removals of partials. Looks like you're mostly done? We've missed the branch point though, so a day wont hurt.
On 2012/09/18 07:26:00, nduca wrote: > I'm fine if you can land your removals of partials. Looks like you're mostly > done? We've missed the branch point though, so a day wont hurt. ok, I'll land that change tomorrow morning. just need to update webkit_compositor_bindings_unittests before it can land.
https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... File cc/CCTextureUpdateController.cpp (right): https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:18: static const size_t pixelsPerTexture = 256 * 256; On 2012/09/18 05:30:35, David Reveman wrote: > We could get rid of this awkward constant if partial updates were not > considered. Done. https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:21: static const size_t textureUpdatesPerFullTickMin = 12; I'd like to get rid of it too. https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:46: (size_t) floor(textureUpdateTickRate * texturesPerSecond)); On 2012/09/18 05:30:35, David Reveman wrote: > Use static_cast here instead of c-style cast. Done. https://codereview.chromium.org/10916292/diff/24001/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:51: TRACE_EVENT0("cc", "CCTextureUpdateController::updateTextures"); On 2012/09/18 05:30:35, David Reveman wrote: > You wouldn't have to touch this function at all if we stopped measuring partial > updates. Done. https://codereview.chromium.org/10916292/diff/24001/cc/ThrottledTextureUpload... File cc/ThrottledTextureUploader.cpp (right): https://codereview.chromium.org/10916292/diff/24001/cc/ThrottledTextureUpload... cc/ThrottledTextureUploader.cpp:100: } On 2012/09/18 05:30:35, David Reveman wrote: > nit: prefer if you keep the blank line you removed here Done. https://codereview.chromium.org/10916292/diff/24001/cc/test/CCTiledLayerTestC... File cc/test/CCTiledLayerTestCommon.h (right): https://codereview.chromium.org/10916292/diff/24001/cc/test/CCTiledLayerTestC... cc/test/CCTiledLayerTestCommon.h:139: virtual double estimatedPixelsPerSecond() OVERRIDE { std::numeric_limits<double>::max(); }; On 2012/09/18 05:30:35, David Reveman wrote: > nit: unnecessary semicolon Done.
Awesome! LGTM with nits. https://codereview.chromium.org/10916292/diff/22004/cc/CCTextureUpdateControl... File cc/CCTextureUpdateController.cpp (right): https://codereview.chromium.org/10916292/diff/22004/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:19: static const size_t maxPartialTextures = 12; maybe partialTextureUpdatesCount or partialTextureUpdatesMax instead? https://codereview.chromium.org/10916292/diff/22004/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:43: return std::max(static_cast<size_t>(1), texturesPerTick); you can get rid of the static_cast now, right?
https://codereview.chromium.org/10916292/diff/22004/cc/CCTextureUpdateControl... File cc/CCTextureUpdateController.cpp (right): https://codereview.chromium.org/10916292/diff/22004/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:19: static const size_t maxPartialTextures = 12; Going with partialTextureUpdatesMax. https://codereview.chromium.org/10916292/diff/22004/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:43: return std::max(static_cast<size_t>(1), texturesPerTick); There's a compile error without the static_cast, so it has to stay ugly. Alternatively, I could use the following, but I don't think it's as clear as using max(): return texturesPerTick ? texturesPerTick : 1;
https://codereview.chromium.org/10916292/diff/22004/cc/CCTextureUpdateControl... File cc/CCTextureUpdateController.cpp (right): https://codereview.chromium.org/10916292/diff/22004/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:43: return std::max(static_cast<size_t>(1), texturesPerTick); On 2012/09/18 21:09:31, Brian Anderson wrote: > There's a compile error without the static_cast, so it has to stay ugly. > Alternatively, I could use the following, but I don't think it's as clear as > using max(): > return texturesPerTick ? texturesPerTick : 1; is "1u" as the first parameter enough to escape the ugly?
https://codereview.chromium.org/10916292/diff/22004/cc/CCTextureUpdateControl... File cc/CCTextureUpdateController.cpp (right): https://codereview.chromium.org/10916292/diff/22004/cc/CCTextureUpdateControl... cc/CCTextureUpdateController.cpp:43: return std::max(static_cast<size_t>(1), texturesPerTick); On 2012/09/18 21:13:00, jamesr wrote: > On 2012/09/18 21:09:31, Brian Anderson wrote: > > There's a compile error without the static_cast, so it has to stay ugly. > > Alternatively, I could use the following, but I don't think it's as clear as > > using max(): > > return texturesPerTick ? texturesPerTick : 1; > > is "1u" as the first parameter enough to escape the ugly? Nope, but 1LU is.
I'm having trouble getting try bots to run. The "Try More Bots" link result in failures without any information about why. And if I run "git try" I cannot authenticate myself, even though I use the same password I use to log into this code review. Anyone know what might be going on?
On 2012/09/18 21:32:33, Brian Anderson wrote: > I'm having trouble getting try bots to run. The "Try More Bots" link result in > failures without any information about why. And if I run "git try" I cannot > authenticate myself, even though I use the same password I use to log into this > code review. > > Anyone know what might be going on? I'm not sure why "Try more bots" dislikes you. For the authentication on the command line, you want to use your svn password not your codereview password: http://chromium-access.appspot.com/. If you don't have SVN write access that would explain both issues. We can fix that, though.
On 2012/09/18 21:35:31, jamesr wrote: > On 2012/09/18 21:32:33, Brian Anderson wrote: > > I'm having trouble getting try bots to run. The "Try More Bots" link result in > > failures without any information about why. And if I run "git try" I cannot > > authenticate myself, even though I use the same password I use to log into > this > > code review. > > > > Anyone know what might be going on? > > I'm not sure why "Try more bots" dislikes you. For the authentication on the > command line, you want to use your svn password not your codereview password: > http://chromium-access.appspot.com/. > > If you don't have SVN write access that would explain both issues. We can fix > that, though. "Try more bots" dislikes me too even though I have SVN write access.
Looks like the try server was just restartd? On Tue, Sep 18, 2012 at 2:35 PM, <jamesr@chromium.org> wrote: > On 2012/09/18 21:32:33, Brian Anderson wrote: > >> I'm having trouble getting try bots to run. The "Try More Bots" link >> result in >> failures without any information about why. And if I run "git try" I >> cannot >> authenticate myself, even though I use the same password I use to log into >> > this > >> code review. >> > > Anyone know what might be going on? >> > > I'm not sure why "Try more bots" dislikes you. For the authentication on > the > command line, you want to use your svn password not your codereview > password: > http://chromium-access.**appspot.com/<http://chromium-access.appspot.com/> > . > > If you don't have SVN write access that would explain both issues. We can > fix > that, though. > > https://codereview.chromium.**org/10916292/<https://codereview.chromium.org/1... >
> For the authentication on the > command line, you want to use your svn password not your codereview password: > http://chromium-access.appspot.com/. There we go.
Can the following try bot errors be ignored? 1) A few failures are with "check_deps2submodules" and look like: http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds... 2) PanelAndDesktopNotificationTest.ExpandAndCollapsePanel is failing with errors that look unrelated. (I'm re-running linux-rel now to verify.): http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/5865...
On 2012/09/18 22:38:18, Brian Anderson wrote: > Can the following try bot errors be ignored? > > 1) A few failures are with "check_deps2submodules" and look like: > http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds... > > 2) PanelAndDesktopNotificationTest.ExpandAndCollapsePanel is failing with errors > that look unrelated. (I'm re-running linux-rel now to verify.): > http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/5865... Yes. For the first one, if it generated an email to you reply to that email and say that the bot or the script the bot runs is busted.
Yeah looks ready to commit.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/brianderson@chromium.org/10916292/24022
Change committed as 157473 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
