Skip to content

Conversation

@michielbdejong
Copy link
Member

No description provided.

@michielbdejong
Copy link
Member Author

Cache ACL documents and profiles of owners therein for 5 minutes.
Brings the test I did in #1443 (comment) down from 1 minute to 1 second.

@angelo-v
Copy link
Contributor

That means, if the cache is not hit, it still takes 5 minutes?

@michielbdejong
Copy link
Member Author

That means, if the cache is not hit, it still takes 5 minutes?

No, if the cache is not hit, it will still fetch the document only once instead of 18 times, so it will still be something like 18 times faster.
If the cache is hit it will be up to 1000 times faster, depending on the ratio between CPU and network speed.

Copy link
Contributor

@timbl timbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Lots of other cache strategies are possible, but let's put this in now to get over the performance problem we have now.
Not sure also what good coding practice is for where to declare this server-wide variable.

@michielbdejong
Copy link
Member Author

where to declare this server-wide variable.

Yes, good question. It doesn't need to be a singleton though, it doesn't matter if it exists twice, once per execution thread, or even once per http request, just that the less unique it is, the less effective it is. Having it as an in-memory global in the lib/acl-checker.js file feels ok to me now, we can always move it around if our insights change.

Looks good to me. Lots of other cache strategies are possible, but let's put this in now to get over the performance problem we have now.

Thanks!

@michielbdejong michielbdejong merged commit 566ea86 into master Aug 19, 2020
@michielbdejong michielbdejong deleted the acl-checker-cache branch August 19, 2020 18:26
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.

4 participants