-
Notifications
You must be signed in to change notification settings - Fork 150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a cache for self profiles #1747
Conversation
8e36f9f
to
2cb1ede
Compare
2cb1ede
to
779deae
Compare
Are we dropping so much data because we don't actually need it, or something else? Basically - maybe we should also/only put the 50 kb version in s3 in the first place? |
Also, how much is the 0.5-1s due to network? i.e., should we expect the actual server to be much faster here? |
It's much smaller, because it's postprocessed and aggregated. Basically, what we store on the server is the input of this function, and what we compute (and cache in this PR) is the output of this function. I'm not sure if we'll ever need anything else than the postprocessed results (we don't currently). However, we let users download the raw self profile files (I'm not sure if anyone uses it, and if yes, if they need anything else than the analyzeme results).
Locally, downloading takes ~0.5s, and extraction and analysis takes ~0.3s. I don't know what happens on the server, because I don't currently see logs, but when you open the detailed result page, it takes 1-2s to load the self-profile endpoint, so the results seem to be consistent with local execution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with merging this but I think a good follow-up would be to explore adding metrics for the cache and perhaps generally our requests so we can (or at least I can) monitor whether we're seeing good cache hit ratios or it's generally pretty useless and so not worth the complexity.
See https://perf.rust-lang.org/perf/metrics for the current list of metrics which we collect.
I will add metrics in a later PR. There is an immediate benefit to this PR: when you open the detailed results page and want to change the sorting columns in the table, it takes a few seconds for the page to reload. After this PR, it should be instant. |
The self-profiles are downloaded from S3 on each request to the detailed results page. Each file has ~12 MiB, downloading and analysing takes ~0.5-1s, and for the detailed page we download two of these profiles.
After download and post-processing, the profile only takes about ~50 KiB in memory, so I think that it makes sense to cache these profiles. This PR adds such a cache, with 1000 entries (using a LRU cache), so that it should take ~50 MiB in memory at most.
If you don't think that a cache is necessary, we could at least keep the first commit, which refactors profile download and should make it easier to reuse it for other use-cases (e.g. for doing more analyses in the benchmark detail endpoint).