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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Performance: 馃殌 Change ParseUrl to use LRUCache #13892
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 馃摑 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed it! |
CLAs look good, thanks! |
bbed01b
to
b08494a
Compare
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.
@cjdeleon62 Excellent PR! Just a small request before we merge.
test/functional/test-url.js
Outdated
const a1 = parseUrl(url); | ||
// cache 99 more urls in order to reach max capacity of LRU cache: 100 | ||
for (let i = 0; i < 100; i++) { | ||
parseUrl(`${url}-${i}`); |
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.
expecting result of parseUrl to be same as a1 would be a nice addition to ensure cache part is working as well.
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.
@aghassemi Ah right, that definitely makes sense. I'll add that in
b08494a
to
b6dffaf
Compare
cef8cd2
to
06d9b97
Compare
@aghassemi wondering if you can review the changes I made after your comments 馃槂 |
LGTM, Merged. Thanks for contributing @cjdeleon62! |
@aghassemi thanks! |
Improves the performance of ParseUrl by allowing the function to keep track of the urls using an LRU cache with a max capacity of 100.
issue : #13280