-
Notifications
You must be signed in to change notification settings - Fork 61
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
Thread safe version of validateShapes #139
base: master
Are you sure you want to change the base?
Conversation
… and threaded the contents inside the shapes loop.
… running validateShapes
Thanks. Note I will be traveling for the next 3 weeks and may not get into this before I am back, doing only light work. |
Sounds good. Enjoy your break! |
Hi @beaudet, I finally had a breather to look into this PR. I see nothing fundamentally wrong here. And I can certainly believe that this will speed up most validation runs when run as a stand-alone application (e.g. command line tool). My practical problem is that the code base here also needs to work in our product, which is a multi-user server. This complicates matters quite a bit. For example we would need to test the potential improvements in all kinds of contexts, i.e. with different number of active users. And I can, of course, not simply rely on Runtime.getRuntime().availableProcessors() when the true number of available processes would be quite different. I don't think I can simply give away all resources to a single user, e.g. what happens if someone starts a lengthy validation and then others try to do the same - the first user may block too many resources. I am frankly not experienced enough with such scenarios to be able to judge on the benefits, but I can certainly see the immediate workload that we would have here to make this work correctly and safely under all circumstances of our product. BTW have you done further experiments to measure the benefits? If this is indeed sufficiently beneficial, I guess you could keep your fork alive while I am unable to merge it into our (main) product branch. |
Uss the |
Happy to make that change if @HolgerKnublauch agrees that's a good approach. Another option might be to add additional signatures or options for validateShapes / validateAll that accepts an executor. |
Yes thanks for the pointer, @afs . ForkJoinPool looks promising. I have created a Jira ticket on our internal tracker to investigate this whole topic, so I would hopefully find more time in the coming months to help here. |
Swapped out WeakHashMap with a Guava variant and synchronized a number of methods and blocks that add validation result nodes to models. This seems to enable reliable multi-threaded validation within validateShapes and the JUnit tests run about 25% faster now so hopefully performance improvements will be even more noticeable with larger numbers of shapes and graphs.