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

fix: resolve LRU conflicts, cache loss and premature engine breaking change #2988

Merged
merged 5 commits into from
Sep 11, 2024

Conversation

wellwelwel
Copy link
Collaborator

@wellwelwel wellwelwel commented Aug 27, 2024

Fixes #1885, Fixes #1953, Fixes #1965, Fixes #1970, Fixes #2001, Fixes #2537, Fixes #2619, Fixes #2752.


Important

This PR replaces the lru-cache.

  • Resolves all issues related to "LRU is not a constructor" since it is fully compatible with both CJS and ESM environments, from Node.js 8 onwards using zero pollyfill (it works even in older browsers).

    #1885, #1965, #1970, #2001, #2537, #2619

  • The same solution above also fixes the premature breaking change, requiring users to upgrade on Node.js 16.14 (dropping runtime versions isn't a problem, but it should be done in major releases)

    #1953

  • When setMaxParserCache is used, MySQL2 overwrites/loses all previous cache (even for a higher size). After this PR, the cache is dynamically resized.

    #2752, #2757

⚡️ This brings a minimally performance improvement and uses a little less CPU (but not such a significant difference).


A quick explanation

The motivation 🧑🏻‍🔬

The two most used projects (lru-cache and quick-lru) that offer the most complete solutions usually dropping Node.js versions (and it's fine). But this situation requires maintainers to also release a major version or continue to use a discontinued/old version.

Based on 7 of 8 linked issues, it started to cause a cascading reaction when each different package made a different decision, causing conflicts when installing the same package in different major versions of the same root project (it's not only a MySQL2 problem).

The solution 💡

I noticed that there was no need for literally ANY pollyfill (even after compilation) to run a LRU cache with even a little more performance (specially for Bun), which is where I decided bringing this dependency to MySQL2.


Recognizing the importance of MySQL2, I will include some details about the reliability of the project in the next comment (please don't associate this with any kind of promotion).

Since this PR replaces lru-cache, I'll understand perfectly if you don't like it — please feel free to close it 🙋🏻‍♂️

@wellwelwel wellwelwel linked an issue Aug 27, 2024 that may be closed by this pull request
@wellwelwel
Copy link
Collaborator Author

wellwelwel commented Aug 27, 2024

I would like to emphasize that lru-cache is the architecture basis of the proposed project, while methods/features and its functionalities are inspired by quick-lru 🤝

The real advantage is an extensive compatibility and the flexibility to dynamically resize the cache.

That said, "lru.min" was primarily optimized based on the use of MySQL2 and is 100% covered throughout E2E based tests:

  • Coverage

About performance, the benchmarks are created by comparing 1,000,000 runs through a maximum cache limit of 100,000, getting 333,333 caches and deleting 200,000 keys 10 consecutive times (isolated processes for each package), clearing the cache for each run.

  • CommonJS results example (In MySQL2's case)
# Time:
  lru.min:    242.86ms
  lru-cache:  264.30ms

# CPU:
  lru.min:    280118.00µs
  lru-cache:  310327.20µs

If someone wants to take a closer look, see how it happens here.


In practice, the main difference is how the cache is created (function instead of class):

- new LRU({ max })
+ createLRU({ max })

If there's an interest in merging this PR, that's the repository: lru.min.

@sidorares, I'm also completely open to include you as a maintainer (npm and GitHub) if you consider it ideal.

Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.13%. Comparing base (ddc299a) to head (babca6c).
Report is 32 commits behind head on master.

Files with missing lines Patch % Lines
lib/parsers/parser_cache.js 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2988   +/-   ##
=======================================
  Coverage   88.13%   88.13%           
=======================================
  Files          71       71           
  Lines       12889    12889           
  Branches     1352     1352           
=======================================
  Hits        11360    11360           
  Misses       1529     1529           
Flag Coverage Δ
compression-0 88.13% <88.88%> (ø)
compression-1 88.13% <88.88%> (ø)
tls-0 87.55% <88.88%> (ø)
tls-1 87.89% <88.88%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wellwelwel wellwelwel marked this pull request as ready for review August 27, 2024 11:56
@wellwelwel wellwelwel added refactor dependencies Pull requests that update a dependency file labels Aug 27, 2024
@wellwelwel wellwelwel changed the title fix: resolve LRU conflicts and premature engine breaking change fix: resolve LRU conflicts, cache loss and premature engine breaking change Aug 30, 2024
@robert-pitt-foodhub
Copy link
Contributor

I upgrade several hundred lambda function functions to a version of mysql2 that included this cache change and I observed no notable issues while testing, We have over 30+ developers / testers working in this environment so I am happy to leave for 24 hours to dogfood this.

So far no issues observed, even the deployment / CI phase went as expected without the LRU Constructor cache error.

@wellwelwel
Copy link
Collaborator Author

I upgrade several hundred lambda function functions to a version of mysql2 that included this cache change and I observed no notable issues while testing, We have over 30+ developers / testers working in this environment so I am happy to leave for 24 hours to dogfood this.

So far no issues observed, even the deployment / CI phase went as expected without the LRU Constructor cache error.

Thank you, @robert-pitt-foodhub 🤝

@robert-pitt-foodhub

This comment was marked as off-topic.

@sidorares
Copy link
Owner

sorry for lack of input @wellwelwel , been offline for a week. Happy to see this merged!

@wellwelwel wellwelwel merged commit 2c3c858 into sidorares:master Sep 11, 2024
72 checks passed
@wellwelwel wellwelwel deleted the lru branch September 11, 2024 03:20
@onhate
Copy link

onhate commented Sep 12, 2024

ow nice!!! now I can upgrade to version 3 finally!!! thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment