Synchronize the access to LRUCache

[ramele1907]

• a wizard that is creating multiple cdt projects encounters ConcurrentModificationException

[jonahgraham]

Hi @ramele1907 - thanks for the PR. Unfortunately the code as provided doesn't compile. See build ouput for more details.

findPathInProjectCache has some more complicated semantics as it is a map of maps, so the inner map probably needs synchronizing too. e.g. add additional Collections.synchronizedMap around the new LRUCache in findPathInProject.

Because # of reads far outweigh number of writes and you indicated that multiple projects are being created simultaneously, did you consider concurrency instead of synchronizing all accesses to the map? Those caches were added in 9e7b5be to dramatically speed up the code, and I think that adding synchronized around each read access may have an unintentional slowdown. If you can demonstrate that synchronizedMap doesn't (seriously) negatively affect performance (see the lined commit for some stats) then we don't need to consider concurrency.

It would help if there is a way to reproduce the problem so that we can see the problem in action.

[jonahgraham]

@ramele1907 Would you like to have a look at this and see if you can resolve the compile errors and other comments I made in my preliminary review #978 (comment)

[ramele1907]

@jonahgraham I resolved the compile errors. I will try to use concurrency instead of synchronization to avoid performance issues.

Regarding the reproduction topic I will try to create a mock plugin with a wizard that creates multiple projects.

[jonahgraham]

@jonahgraham I resolved the compile errors.

Great - I will set the build off running. On your next update please update to the latest HEAD of the main branch.

I will try to use concurrency instead of synchronization to avoid performance issues

I'm pleased you found concurrency easy enough to do. Did you calculate the performance issue? Was it significant?

Regarding the reproduction topic I will try to create a mock plugin with a wizard that creates multiple projects.

Wonderful! I recently did something similar by creating an example CMake project wizard that isn't shipped to customers, but demonstrates how to make custom wizards. See https://github.com/eclipse-cdt/cdt/blob/main/NewAndNoteworthy/CDT-12.0.md#cmake-support-easier-for-extenders-and-isvs

[jonahgraham]

Also did you see this comment from my original review #978 (comment)?

findPathInProjectCache has some more complicated semantics as it is a map of maps, so the inner map probably needs synchronizing too. e.g. add additional Collections.synchronizedMap around the new LRUCache in findPathInProject.

I am relying on you to verify that my statement is true (that the inner maps need addressing)

[github-actions]

Test Results

   600 files  ±0     600 suites  ±0   13m 29s ⏱️ +3s
10 207 tests ±0  10 184 ✅ ±0  23 💤 ±0  0 ❌ ±0 
10 245 runs  ±0  10 222 ✅ ±0  23 💤 ±0  0 ❌ ±0 

Results for commit 4167d94. ± Comparison against base commit f0c74ed.

♻️ This comment has been updated with latest results.

[ramele1907]

findPathInProjectCache has some more complicated semantics as it is a map of maps, so the inner map probably needs synchronizing too. e.g. add additional Collections.synchronizedMap around the new LRUCache in findPathInProject.

I agree, the access to findPathInProject might also lead to a ConcurrentModificationException.

I'm pleased you found concurrency easy enough to do. Did you calculate the performance issue? Was it significant?

I saw no performance gain in my product after switching from synchronization to concurrency. I think the computation of the values was the bottleneck addressed in 9e7b5be

Wonderful! I recently did something similar by creating an example CMake project wizard that isn't shipped to customers, but demonstrates how to make custom wizards. See https://github.com/eclipse-cdt/cdt/blob/main/NewAndNoteworthy/CDT-12.0.md#cmake-support-easier-for-extenders-and-isvs

Thank you for your suggestion! I will come back with updates on this topic.

[jonahgraham]

@ramele1907 I see that you did a merge, but that didn't ended up getting pushed as the latest commit in this PR. I have merged and rebased your change onto the latest HEAD commit so that the workflows can run.

[jonahgraham]

Thank you for your suggestion! I will come back with updates on this topic.

We are nearly out of time for CDT 12.0. Are there more updates coming on this topic?