Skip to content
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

Cache the base path of bullet_train when accessing documentation #314

Merged
merged 6 commits into from
Aug 23, 2023

Conversation

gazayas
Copy link
Contributor

@gazayas gazayas commented Jun 8, 2023

I looked into the TODO in the DocumentationSupport module, and although this doesn't fix it, it gives us a speed boost by caching the bundle show bullet_train output.

I can't think of another way right now to access the docs from the gem in the context of a Bullet Train app besides linking to each raw file here on GitHub, but I feel like that would be slower anyways. I'm personally okay with bundle show bullet_train initially if we keep this cache, but I didn't delete the TODO just in case there's another way we want to approach this problem.

@jagthedrummer
Copy link
Contributor

@gazayas I'm curious if we've seen problems crop up due to the lack of caching. I'm a little concerned that there's no timeout on this cache and so once it's cached it could potentially stay around forever. I'm thinking we'd want it to change if a new version of the gem has been installed to a different place.

@gazayas
Copy link
Contributor Author

gazayas commented Aug 9, 2023

@jagthedrummer Good point, I didn't think about version discrepancies, and if we wanted to check for a new version to compare with the cache we'd have to run bundle show again anyways... So, I went ahead and updated the cache to expire in 30 minutes. Do you think that's a good amount of time?

@jagthedrummer
Copy link
Contributor

@gazayas I'm mostly curios if there are documented problems here without the caching. And/or how much of a speed difference does it make? Basically I'm wondering if the juice is worth the squeeze on this one. (With cache invalidation being one of the hard things, and all that.)

@gazayas
Copy link
Contributor Author

gazayas commented Aug 9, 2023

@jagthedrummer I have always felt like accessing the docs on the live website can be a little slow especially due to bundle show being called, but I could be wrong and I don't actually have concrete benchmarks to prove my case 😬 So I'll see if I can come up with some, and we can decide what to do from there!

@jagthedrummer
Copy link
Contributor

jagthedrummer commented Aug 23, 2023

@gazayas I just did some very unscientific bench-marking on this and... you were right! Kinda.

It turns out that calling bundle show on every request is kinda slow. When we do it on every request I was seeing local times for /docs/* paths that were consistently in the 220-230ms range.

But, it seems like making a network round trip to the cache, even locally, is close to equally slow. Using your Rails.cache fix I was seeing times consistently in the 190-200ms range.

By memoizing the value in a constant I was seeing times consistently in the 10-20ms range.

Memoizing also has the nice side effect of letting us sidestep the question about how long to cache the value. Every time the server (or any process) is restarted the value will be re-evaluated, which will take care of my concern about us caching an out-of-date path.

@jagthedrummer jagthedrummer merged commit b592dd7 into main Aug 23, 2023
1 check passed
@jagthedrummer jagthedrummer deleted the cache-docs-base-path branch August 23, 2023 19:32
@jagthedrummer
Copy link
Contributor

After updating the demo site and deploying it, this PR has made a big difference!

Before:
CleanShot 2023-08-23 at 15 10 06

After:
CleanShot 2023-08-23 at 15 10 16

Good call on trying to optimize it, @gazayas!

@gazayas
Copy link
Contributor Author

gazayas commented Aug 23, 2023

@jagthedrummer Thanks for optimizing this one, and sorry for the delay! I'll be sure to try to use this technique if something similar shows up again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants