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

Improve performance via Triemap in workspace #1406

Merged
merged 3 commits into from
Oct 15, 2024

Conversation

pderaaij
Copy link
Collaborator

The compromised version of #1391.

@pderaaij
Copy link
Collaborator Author

I am not sure why this test case is suddenly failing. I have to look at this with some fresh brains later this week. Always open for suggestions.

@pderaaij
Copy link
Collaborator Author

@riccardoferretti On my local machine I have a passing test, but in CI I can't make it work. Could you have a look in your environment and see what happens? Perhaps you can spot a problem, I am missing it right now.

@riccardoferretti
Copy link
Collaborator

(from Discord)

yeah I saw that. Weird thing was that I was able to reproduce it locally, then made a small change to the content of the definition of fileC (a few lines above) as I was wondering if it was some odd caching issue. (the reason was that the test fails for 5 chars, which is the difference between the wikilink in fileB and fileC - long shot but I tried, and it "kinda" fixed the immediate issue, the puzzling question became what is causing that). After the change the test passed, and the oddest thing was that after reverting the change it continued to pass.
basically in the definition of fileC in the test, I made the string a bit longer ("File C is also connected to" -> "File C is also connected also to")

@riccardoferretti
Copy link
Collaborator

Odd, but do you wanna try the same thing and see if it passes CI?

@pderaaij
Copy link
Collaborator Author

Well... adding the text didn't help, unfortunately. But, it did help me to think about the problem differently. What I found is that the references are returned in no particular order. Depending on the random file name, the first wikilink might come in as first reference or as second. If the latter happens, that will fail the test. The random file names explain the flakiness of the test.

I expect this is an issue since this change. As TrieMap does not hold the insertion order, the issue manifests during the build. For other places I expect this not to be a problem as they are sorted in their view/UI panel.

Other than that, this PR is ready for review and testing. Just did a test with the 10000s folder, the activation log states:

Foam Logging: info
[info - 19:51:05] Starting Foam
[info - 19:51:07] Loading from directories:
[info - 19:51:07] - /Users/paulderaaij/projects/interconnected-markdown/Markdown/10000s/10000
[info - 19:51:07]   Include: **/*
[info - 19:51:07]   Exclude: **/.foam/**,**/.vscode/**/*,**/_layouts/**/*,**/_site/**/*,**/node_modules/**/*,**/.git,**/.svn,**/.hg,**/CVS,**/.DS_Store,**/Thumbs.db
[info - 19:51:14] Workspace loaded in 7253ms
[info - 19:51:17] Graph loaded in 2227ms
[info - 19:51:17] Tags loaded in 106ms
[info - 19:51:17] Loaded 10001 resources

Compared to 25+ seconds that the current release needs for loading that workspace.

@riccardoferretti
Copy link
Collaborator

Ahh makes perfect sense, good find!

Copy link
Collaborator

@riccardoferretti riccardoferretti left a comment

Choose a reason for hiding this comment

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

Amazing, thanks @pderaaij !

@riccardoferretti riccardoferretti merged commit 5a6ef64 into foambubble:master Oct 15, 2024
3 checks passed
@pderaaij pderaaij deleted the triemap-performance branch October 16, 2024 08:28
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