lazy_static rhai Engine #112
Conversation
Codecov Report
@@ Coverage Diff @@
## master #112 +/- ##
==========================================
- Coverage 86.97% 86.95% -0.03%
==========================================
Files 19 19
Lines 2834 2836 +2
==========================================
+ Hits 2465 2466 +1
- Misses 369 370 +1
Continue to review full report at Codecov.
|
src/enforcer.rs
Outdated
| if !self.enabled { | ||
| return Ok(true); | ||
| } | ||
|
|
||
| let mut engine = Engine::new(); | ||
| let mut engine = ENGINE.write().unwrap(); |
There was a problem hiding this comment.
The original version of enforce is lock-free, will this change makes enforce can't handle concurrent request because every request have to wait for the write lock.
There was a problem hiding this comment.
I'm not sure if it matters, I can't get it to panic even using RwLock::try_write in either an async spawned context or on a std thread. The only way to pass the Enforcer to a thread or an async_std::spawn is to wrap it in some type of sync control. I'm pushing another commit mostly to show the test I wrote for this. If you can think of any other ways to pass this into concurrent execution it'd be great to add them to the test.
This also reminds me I was wondering about using async_std::sync::RwLock/Mutex instead of std's RwLock/Mutex. There is a discussion about this sort of here
There was a problem hiding this comment.
If we do have only one enforcer, it's ok to use lazy_static, but I would say if the users created multiple enforcers. It will cause problems because basically they will share the same Engine and the more enforcers we have, the worser result we get. => This is also why I removed lazy_static event emitter from the code
@xcaptain @DevinR528 I think inline engine, scope will improve bench about 15%, but yes I didn't succeed to implement it yesterday due to lifetime issue.
May try again in the future
@DevinR528 You are right we shouldn't use std::sync::RwLock to wrap a type which implemented many async functions. Because .await requires Send and std::sync::RwLockGuard is not send. That causes also difficutiles when I try to implement a clonable SyncedEnforcer, which is basically Arc<RwLock<Enforcer/CachedEnforcer>>. This maybe out of topic
There was a problem hiding this comment.
Instead of just one Engine could you have a lazy_static Arc<Mutex<Vec<Engine>>> and every time Enforcer::new is called you push a new Engine into the vec and assign an index for that Enforcer instance? I'm going to make a somewhat educated guess that the lifetime issues may be insurmountable 😢, basically the lifetime of the &str's passed to Scope::push_constant become the lifetime of the &mut self borrow for Enforcer::enforce and there doesn't seem to be a way to say a lifetime that is smaller than the lifetime of the Enforcer struct which is what you need.
There was a problem hiding this comment.
@DevinR528
Arc<Mutex<Vec<Engine>>> still require write access, it means when one enforcer is enforcing, there won't be any other enforcer can acquire a engine.
I believe rhai::Engine 's lifetime is to for the registered functions.
There was a problem hiding this comment.
Ahh right the vec won't work at all. I had one more idea creating a lockless AtomicPtr struct that has a flag for when the Engine is in use and either starts we a few Engines or adds to a vec as needed.
|
closed as solved in #117 , we dont need to share scope because it took only 5ns to create |
Relates to #109
Add
block_onwhich tests a future with no runtime overhead.use
lazy_staticto only create oneEngineperEnforcernot on everyenforece()call.