Speed up presubmit by caching code-owners check [chromium/tools/depot_tools : main]
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
We might also want to do this for IsCodeOwnersEnabledOnHost, but I figured I'd get buy in on this CL before doing that.
Related details
- Yiwei Zhang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiwei Zhang (Gerrit)
Yiwei Zhang added 1 comment![]( "Open in Gerrit")
I think caching is a good idea given the enablement of code owners plugin should never changes. However, I'm wondering if there's any particular reason to cache the result in git config rather than in memory (e.g. using @functools.lru_cache decorator) ?
Related details
- Josiah Kiehl
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
I think caching is a good idea given the enablement of code owners plugin should never changes. However, I'm wondering if there's any particular reason to cache the result in git config rather than in memory (e.g. using @functools.lru_cache decorator) ?
I was thinking it would be nice to persist on disk somewhere, and the git config is a convenient place to store it per-repository. If we store it in memory, we'll have to make the call once every presubmit time we run a script?
Related details
- Yiwei Zhang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yiwei Zhang (Gerrit)
Yiwei Zhang added 1 comment![]( "Open in Gerrit")
Josiah KiehlI think caching is a good idea given the enablement of code owners plugin should never changes. However, I'm wondering if there's any particular reason to cache the result in git config rather than in memory (e.g. using @functools.lru_cache decorator) ?
I was thinking it would be nice to persist on disk somewhere, and the git config is a convenient place to store it per-repository. If we store it in memory, we'll have to make the call once every presubmit time we run a script?
That also might not work well with non-git environment. Stepping back a little bit, it looks like we did cache the result at [the caller](https://source.chromium.org/chromium/chromium/tools/depot_tools/+/main:presubmit_support.py;l=598-602;drc=2e88a3f08bd8c4a0014eae82729beca935f7f188). Therefore, it should just make the call once for each presubmit run. Are you saying that the call itself could just take 1.5s?
Related details
- Josiah Kiehl
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
Josiah KiehlI think caching is a good idea given the enablement of code owners plugin should never changes. However, I'm wondering if there's any particular reason to cache the result in git config rather than in memory (e.g. using @functools.lru_cache decorator) ?
I was thinking it would be nice to persist on disk somewhere, and the git config is a convenient place to store it per-repository. If we store it in memory, we'll have to make the call once every presubmit time we run a script?
I believe you're right, it only makes one call per process, but it typically takes 0.7s from my glinux cloudtop, though it can take as much as 1.5s in the worst case. This log is in nearly 100% of my `git cl presubmit` runs:
```
Running presubmit commit checks on branch optimize_check_for_commit_objects ...
checking owners took a long time: 0.7s
```
I suppose we do need a different place to put the config for cog/jj environments... I'm not sure where that would be tho, off hand. I will ponder unless you have an idea.
Related details
- Josiah Kiehl
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
Josiah KiehlI think caching is a good idea given the enablement of code owners plugin should never changes. However, I'm wondering if there's any particular reason to cache the result in git config rather than in memory (e.g. using @functools.lru_cache decorator) ?
Josiah KiehlI was thinking it would be nice to persist on disk somewhere, and the git config is a convenient place to store it per-repository. If we store it in memory, we'll have to make the call once every presubmit time we run a script?
I believe you're right, it only makes one call per process, but it typically takes 0.7s from my glinux cloudtop, though it can take as much as 1.5s in the worst case. This log is in nearly 100% of my `git cl presubmit` runs:
```
Running presubmit commit checks on branch optimize_check_for_commit_objects ...
checking owners took a long time: 0.7s
```I suppose we do need a different place to put the config for cog/jj environments... I'm not sure where that would be tho, off hand. I will ponder unless you have an idea.
Another possibility is that the latency is serverside and the server itself is taking this long and needs to cache the response or make it cacheable or something of that nature.
Related details
- Yiwei Zhang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
Josiah KiehlI think caching is a good idea given the enablement of code owners plugin should never changes. However, I'm wondering if there's any particular reason to cache the result in git config rather than in memory (e.g. using @functools.lru_cache decorator) ?
Josiah KiehlI was thinking it would be nice to persist on disk somewhere, and the git config is a convenient place to store it per-repository. If we store it in memory, we'll have to make the call once every presubmit time we run a script?
Josiah KiehlI believe you're right, it only makes one call per process, but it typically takes 0.7s from my glinux cloudtop, though it can take as much as 1.5s in the worst case. This log is in nearly 100% of my `git cl presubmit` runs:
```
Running presubmit commit checks on branch optimize_check_for_commit_objects ...
checking owners took a long time: 0.7s
```I suppose we do need a different place to put the config for cog/jj environments... I'm not sure where that would be tho, off hand. I will ponder unless you have an idea.
Another possibility is that the latency is serverside and the server itself is taking this long and needs to cache the response or make it cacheable or something of that nature.
Digging deeper, I figured out why I was seeing it take 1.5s when the config request usually takes 0.7s.... it's because my benchmark code was capturing the handshake request as well, which happens the first time any request is made, so it's actually 2 requests when the request is not cached, and both of those requests are 0.7s+ in latency, so caching does indeed save a consistent 1.5s per `git cl presubmit` run on my cloudtop, which maths out to a >200x performance improvement.
```
--- Run 1 (Cache Disabled / First Run) ---
BENCHMARK_REQ_START: GET http://chromium-review.git.corp.google.com/a/accounts/self/emails
BENCHMARK_REQ_END: GET http://chromium-review.git.corp.google.com/a/accounts/self/emails took 0.9099s
BENCHMARK_REQ_START: GET http://chromium-review.git.corp.google.com/a/projects/chromium%2Fsrc/code_owners.project_config
BENCHMARK_REQ_END: GET http://chromium-review.git.corp.google.com/a/projects/chromium%2Fsrc/code_owners.project_config took 0.6481s
Result: True
Total time: 1.6346s
--- Run 2 (Cache Enabled / Second Run) ---
Result: True
Total time: 0.0076s
```
Related details
- Yiwei Zhang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Josiah Kiehl (Gerrit)
Josiah Kiehl added 1 comment![]( "Open in Gerrit")
def _AtomicFileWriter(path, mode='w'):This writes to the config directory in favor of git config, per the other comment thread. I would use something like the atomicwrites lib for this, but idk how reasonable it is to add a dependency... and this code is not too involved to maintain, perhaps.
Related details
- Yiwei Zhang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |