Skip to content

AA: trie-based scope for library module #5451

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

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

Conversation

jsjeon
Copy link
Collaborator

@jsjeon jsjeon commented May 23, 2025

...inspired by google/ksp#2361

^KT-71706 fixed

@jsjeon jsjeon requested a review from marcopennekamp May 23, 2025 22:30
@jsjeon jsjeon requested a review from a team as a code owner May 23, 2025 22:30
@jsjeon
Copy link
Collaborator Author

jsjeon commented May 23, 2025

cc @ting-yuan

@marcopennekamp
Copy link
Contributor

Awesome, thanks a lot!


val root = TrieNode()

private val m = mutableMapOf<Pair<TrieNode, String>, TrieNode>().apply {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have a single, global map for the whole trie instead of modeling edges directly in TrieNode? To conserve memory (since we don't have to allocate a String -> TrieNode hash map per node)?

With the current solution, we allocate a new Pair for each access into the map in contains. Maybe that doesn't make a huge difference since we're also splitting the input path and allocating a lot that way, but still. contains should ideally avoid allocations since it can be frequently called.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any benchmarks for this?

It's not going to block the merge, but I wonder how much time we now spend in contains splitting strings.

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