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

Engine ThreadSafety #31

Open
BoasE opened this issue Jul 20, 2018 · 6 comments
Open

Engine ThreadSafety #31

BoasE opened this issue Jul 20, 2018 · 6 comments

Comments

@BoasE
Copy link

BoasE commented Jul 20, 2018

Is the ThreadEngine meant to be thread safe so I can use it as a singleton accross threads?

@pieterderycke
Copy link
Owner

The be honest. I have never tested it to be the case. The main potential issue I see is with the caching of generated delegates. But I think it can definitely be made thread safe without a performance impact.

For which reason do you need to have it thread-safe?

@BoasE
Copy link
Author

BoasE commented Jul 23, 2018

just to avoid unnecessary allocations. e.g. i could have a static engine .

@aviita
Copy link

aviita commented Aug 9, 2021

Thread safety would also make the cache usage more efficient. In my current implementation I am not sharing the CalculationEngine at all between objects. I could definitely do something about this already, but the thread safety would make sense from caching point of view too. Now the MemoryCache and the ConcurrentDictionary it uses are created per CalculationEngine instance.

I have a usa case which relies quite heavily on Jace formula calculations and I can see that lots of time is spent on the DynamicCompiler.BuildFromula().
image

Here converting the cache to be a singleton/thread safe might also do the trick. It could be even configured in the JaceOptions if the cache is to be handled as singleton or not. Another option would be to use .Net MemoryCache directly and rely on it's cleanup and thread safety mechanisms. Built formulas should be thread safe already as they are just pure functions.

In the my case of which you can see the performance above I have only 32 different formulas, but 1168 instances of the engine.

@aviita
Copy link

aviita commented Aug 9, 2021

I could improve my code by creating the Engine per user, so I would be ensured to have no concurrency issues, but even that approach leaves two problems.

  1. There is a performance penalty for new user sessions.
  2. There is memory overhead to even in having the cache per user.

I my case I think I could even have all the formulas cached as I have maximum of ~1900 formulas, but I think the real number is even much lower as that 32 different formulas for one use case already has almost maximum complexity. I have roughly 60 other cases for the formulas and they are much simpler mostly.

@aviita
Copy link

aviita commented Aug 9, 2021

Ah, and I did notice I did invalid lookup to the .Net documentation. .Net Standard does not have the MemoryCache, so that must be why ConcurrentDictionary is used. So looking now more into the approach of making it static.

@aviita
Copy link

aviita commented Aug 9, 2021

As for the thread safety in general. It would seem to me that ConstantRegistry for one would need work to be thread safe. And FunctionRegistry could potentially be even more problematic if shared. But if ommitting use of those two features it could be possible to make the engine a singleton. This is somewhat risky though. Maybe in that case there should be separate implementation of the CalculationEngine, something like ConcurrentCalculationEngine that would ommit the functionality that is not supported.

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

No branches or pull requests

3 participants