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

crdx: Improve performance of calculateConcurrency #124

Closed

Conversation

leblowl
Copy link
Contributor

@leblowl leblowl commented Aug 16, 2024

For a simple graph with about 1000 links, the current version takes about 6 seconds on my machine and the version in this PR takes about 100ms. getSuccessors also appears to be unused now, so I removed that.

@HerbCaudill
Copy link
Member

Hey, Lucas - I haven't been able to reproduce the poor performance of the original code. Using this benchmark, I get pretty similar performance whether I'm using the existing code or your optimized version.

import { getConcurrentLinks } from 'graph/index.js'
import { buildRandomGraph } from 'util/testing/graph.js'
import { bench, describe, expect } from 'vitest'
describe('graphs', () => {
const graph = buildRandomGraph(1000)
bench('getConcurrentLinks', () => {
for (const link of Object.values(graph.links)) {
const concurrent = getConcurrentLinks(graph, link)
expect(concurrent).toBeDefined()
}
})
})

Would you be able to capture the experiments you've done in a benchmark?

@leblowl
Copy link
Contributor Author

leblowl commented Oct 9, 2024

@HerbCaudill Hey, sorry I've been traveling and otherwise pretty busy lately. I will take a look and see.

@leblowl
Copy link
Contributor Author

leblowl commented Jan 12, 2025

@HerbCaudill Just an update, I don't know when/if I will get back around to working on this, so I'm going to close this PR. If I do get to it, I'd need to open a new PR on a new branch from a personal repo anyway.

@leblowl leblowl closed this Jan 12, 2025
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