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

Optimize and benchmark circular references #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

trevorr
Copy link

@trevorr trevorr commented Oct 8, 2024

I added a test for cloning circular references, and it performed very poorly:

$ node benchmark/circles
benchLodashCloneDeep*100: 878.215ms
benchFastCopy*100: 338.467ms
benchNanoCopy*100: 330.19ms
benchRamdaClone*100: 1.878s
benchRfdcCircles*100: 1:03.309 (m:ss.mmm)
benchRfdcCirclesProto*100: 1:03.243 (m:ss.mmm)
benchStructuredClone*100: 862.133ms

I figured the performance was likely due to using Array.indexOf() to handle looking up circular references. I used a Map instead, and the performance improved dramatically:

node benchmark/circles
benchLodashCloneDeep*100: 968.515ms
benchFastCopy*100: 351.399ms
benchNanoCopy*100: 334.315ms
benchRamdaClone*100: 1.945s
benchRfdcCircles*100: 452.196ms // 139x faster
benchRfdcCirclesProto*100: 455.135ms // 139x faster
benchStructuredClone*100: 899.574ms

On the original benchmark (no circular references), there is only a slight degradation on performance when using circles: true. Here's the original code:

node benchmark
benchDeepCopy*100: 360.976ms
benchLodashCloneDeep*100: 855.236ms
benchCloneDeep*100: 498.917ms
benchFastCopy*100: 429.037ms
benchFastestJsonCopy*100: 225.851ms
benchPlainObjectClone*100: 339.774ms
benchNanoCopy*100: 454.609ms
benchRamdaClone*100: 1.368s
benchJsonParseJsonStringify*100: 1.025s
benchRfdc*100: 213.324ms
benchRfdcProto*100: 223.08ms
benchRfdcCircles*100: 237.251ms
benchRfdcCirclesProto*100: 248.533ms
benchStructuredClone*100: 1.164s

And the code from this branch:

node benchmark
benchDeepCopy*100: 362.713ms
benchLodashCloneDeep*100: 845.653ms
benchCloneDeep*100: 495.222ms
benchFastCopy*100: 433.719ms
benchFastestJsonCopy*100: 233.539ms
benchPlainObjectClone*100: 303.27ms
benchNanoCopy*100: 454.411ms
benchRamdaClone*100: 1.369s
benchJsonParseJsonStringify*100: 1.030s
benchRfdc*100: 209.843ms
benchRfdcProto*100: 221.067ms
benchRfdcCircles*100: 309.416ms // 30% slower
benchRfdcCirclesProto*100: 318.529ms // 28% slower
benchStructuredClone*100: 1.143s

Overall, still very competitive, and no longer unusable when many cycles are encountered.

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Collaborator

CI is failing, couyld you fix?

@trevorr
Copy link
Author

trevorr commented Oct 15, 2024

I don't think it has anything to do with my changes. I get the same error on master when I run npm run ci:

==> Uploading reports
/Users/trevor/git/rfdc/node_modules/codecov/lib/codecov.js:212
      } else if (result.split('\n').length !== 2) {
                        ^

TypeError: result.split is not a function
    at /Users/trevor/git/rfdc/node_modules/codecov/lib/codecov.js:212:25
    at /Users/trevor/git/rfdc/node_modules/teeny-request/build/src/index.js:217:17
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Node.js v20.11.1

@trevorr
Copy link
Author

trevorr commented Oct 15, 2024

When I run it locally, result is an object rather than a string: { message: 'Repository not found' }.

@mcollina
Copy link
Collaborator

please rebase, I've removed codecov

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

still lgtm

@mcollina
Copy link
Collaborator

for some odd reasons this is crashing on node v22. Can you take a look?

@trevorr
Copy link
Author

trevorr commented Oct 17, 2024

This also fails on master. It looks like an internal node assert. Perhaps the test dependencies don't support Node 22? When I upgrade tap from 12 to 18+, Node no longer asserts, but the tests fail with messages like "isNot is not a function".

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