Amphtml: Limit UrlCache, make it an LRU.

[aghassemi]

What you will need to know

• Basic Javascript

Background

We have a utility method called parseUrl in url.js that is used to parse a Url string.

This method caches the result of a parse so not to repeat for the same given Url however the cache used for this is unlimited which is no ideal.

Useful Hints

The fix involved changing the cache map to be an LRU (Least Recently Used) cache with a large capacity (100).

Fortunately we have an implementation of LRU cache in lru-cache.js which we can use here.

Step by step

• [ ] Claim this issue by adding a comment below. Please only claim this bug if you plan on starting work in the next day or so. (If you join the AMP Project we'll be able to assign this issue to you after you've claimed it.)
• [ ] If you aren't too familiar with Git/GitHub, see the Getting Started End-to-End Guide for an intro to Git & GitHub, and how to get a copy of the code. You can also refer to the Quick Start Guide for the necessary setup steps with less explanation than the End-to-End guide.
• [ ] Follow the instructions for building AMP.
• [ ] [Create a Git branch](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#create-a-git-branch) for making your changes.
• [ ] [Sign the Contributor License Agreement](https://github.com/ampproject/amphtml/blob/master/CONTRIBUTING.md#contributor-license-agreement) before creating a Pull Request. (If you are contributing code on behalf of a corporation start this process as early as possible.)
• [ ] [Commit your changes](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#edit-files-and-commit-them) frequently.
• [ ] [Push your changes to GitHub](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#push-your-changes-to-your-github-fork).
• [ ] [Create a Pull Request](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#send-a-pull-request-ie-request-a-code-review). Mention @aghassemi and closes Issue #13280 in the description.
• [ ] [Respond to your reviewer's comments](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#respond-to-pull-request-comments) (if any).

Once approved, your changes will be merged. ⚡⚡⚡Congrats on making your first contribution to the AMP Project!⚡⚡⚡ You'll be able to see it live across the web soon!

Thanks, and we hope to see more contributions from you soon.

Questions?

If you have questions ask @aghassemi in this issue or on your Pull Request (if you've created one) or see the How to get help section of the Getting Started guide.

[lannka]

FYI, there is an LRU implemented in utils/lru-cache.js for you to reuse

[jridgewell]

While you're at it, make the LRU's cache a prototype-less object. 😉

[cjdeleon62]

@aghassemi I would like to take a shot at this issue if it is free.

[aghassemi]

@cjdeleon62 Thanks! Assigned to you.

[cjdeleon62]

@aghassemi awesome! I'll start working on it tonight 👍

[cjdeleon62]

Hi @aghassemi, this is my first time contributing to this repo and I just have a question about the unit tests. Some are consistently failing because they are trying to match the updated scrollTop values and the difference between the actual and expected is off by decimal places. Just wondering if this is only happening to me or is this expected to flake sometimes?

[aghassemi]

@cjdeleon62 they are flakes. If your own newly written tests pass, I recommend submitting a PR. All tests would run again on Travis as part of the PR and they are less flaky than localhost runs.

[cjdeleon62]

@aghassemi okay, that is good to know, will submit a PR soon. Thanks!

[cjdeleon62]

13892

[aghassemi]

Closed by #13892