Collapse clean and non clean build requests on clean builders #266
Conversation
This is to address llvm#250. Where build requests were piling up on slower builders because they were clean/don't clean/clean/ don't clean etc. and we could not merge those requests because of that difference. Galina suggested that if we knew that the builder was going to clean the build first anyway, we could merge them. This commit implements that. We do this by first setting a "clean" attribute on the builder's factory object, that we can find later via in the collapse callback. So far only ClangBuilder passes this on but any builder can opt in by copying what it does. In the collapse function when comparing properties we delete the "clean_obj" key from both sets of properties if we know that the builder's factory is always clean. With some logging (not included in this commit) we can see it now collapses a clean/non-clean pair of requests: ``` collapseRequests selfBuildSetProperties: {'scheduler': ('main:clang,clang-tools-extra,compiler-rt,flang,lld,llvm,mlir', 'Scheduler')} otherBuildSetProperties: {'scheduler': ('main:clang,clang-tools-extra,compiler-rt,flang,lld,llvm,mlir', 'Scheduler'), 'clean_obj': (True, 'Change')} Removing clean_obj from build set properties. Build set properties were the same. ``` This has beeen tested by my colleague Omair Javaid on a test build master. Of course we only tested it with one of Linaro's builders, so it's possible we see other side effects in production.
DavidSpickett
requested review from
gkistanova,
vitalybuka,
omjavaid and
luporl
|
Why would we prevent from collapsing clean/non-clean builds? |
|
We already merge: And As the properties matched. Now we want to merge: Assuming unclean merges into clean, and the result is also clean, we could do this for any builder even if it does not clean by default. In current production we wouldn't because the properties don't match. This we can only merge if the builder itself would clean anyway, or if we are able to edit the request after merging to add I will see if I can edit the collapsed request, then this can apply globally not just to builders marked clean. |
|
I'm going to see if it's feasible to modify the request that we collapse into, to address Mehdi's question. And to be clear, Galina has the deciding approval here. |
This is the buildbot function that calls the callback I'm modifying in this PR: https://github.com/buildbot/buildbot/blob/master/master/buildbot/process/buildrequest.py#L66 If we want to collapse unclean -< clean, we'd need to change this callback to return a new request with the But I'd rather not get into changing Buildbot itself (or trying to monkeypatch it)
@joker-eph tell me if I've answered this or not. Might have gone on a sidetrack there. |
|
One sentence answer based on current buildbot behaviour: That is unless we knew that the builder would clean anyway, which is what this PR checks for. |
There was a problem hiding this comment.
LGTM,
But it seems it is necessary to add similar clean=clean to LLDBBuilder.py, PollyBuilder.py, OpenMPBuilder.py and BOLTBuilder.py. Note most builders use UnifiedTreeBuilder which already contains clean property. See the full list of factories with clean=True in llvm-zorg/buildbot/osuosl/master/config/builders.py
Yes, this is exactly what I was looking for, thanks. |
I've cleared the queue for one of our affected builders, https://lab.llvm.org/staging/#/builders/75, and will see how the queue develops. |
|
Is this on staging? |
My apologies, I forgot to merge #257. |
It does not look like it works. https://lab.llvm.org/staging/#/builders/7 |
You patch to make that builder clean was not on the staging yet. It is now. Let's wait for another day to make sure everything is fine. I'll be surprised if it is not, though. |
|
However, since |
Reverts #295 Try again according to #266 (comment)
|
Any estimates when it will reach lab.llvm.org/buildbot? Asking if we need to return workaround #295. |
|
I think this is ready to merge. It could be in production on Friday late night or Staurday morning. |
To take advantage of llvm#266. We use ccache on all our bots so this will not add much overhead. The SVE bots are fast enough that the buildup of build requests is not really a problem, but for consistency I've marked them clean so all our 2 stage builds are clean.
|
Sorry I was away for a long weekend. Linaro's bots also seemed to have no problems with this, didn't break our non-clean builders either. I will merge this now. |
To take advantage of llvm#266. We use ccache on all our bots so this will not add much overhead. The SVE bots are fast enough that the buildup of build requests is not really a problem, but for consistency I've marked them clean so all our 2 stage builds are clean.
To take advantage of #266. We use ccache on all our bots so this will not add much overhead. The SVE bots are fast enough that the buildup of build requests is not really a problem, but for consistency I've marked them clean so all our 2 stage builds are clean.
Fixes #250
Where build requests were piling up on slower builders because they were clean/don't clean/clean/ don't clean etc. and we could not merge those requests because of that difference.
Galina suggested that if we knew that the builder was going to clean the build first anyway, we could merge them. This commit implements that.
We do this by first setting a "clean" attribute on the builder's factory object, that we can find later via in the collapse callback.
So far only ClangBuilder passes this on but any builder can opt in by copying what it does.
In the collapse function when comparing properties we delete the "clean_obj" key from both sets of properties if we know that the builder's factory is always clean.
With some logging (not included in this commit) we can see it now collapses a clean/non-clean pair of requests:
This has beeen tested by my colleague Omair Javaid on a test build master. Of course we only tested it with one of Linaro's builders, so it's possible we see other side effects in production.