feat: engine setup #24
Conversation
This is greatly inspired by Kiesel and SerenetyOS's LibJS JavaScript engines.
Co-authored-by: Aapo Alasuutari <[email protected]>
ghost
self-requested a review
Co-authored-by: Aapo Alasuutari <[email protected]>
| #[derive(Debug)] | ||
| pub struct DeclarativeEnvironment { | ||
| pub outer_env: Option<Environment>, | ||
| pub bindings: HashMap<&'static str, Binding>, |
There was a problem hiding this comment.
comment: I think the keys may benefit from being Value::Strings. After all, often a binding key will also find its way into a map key etc.
There was a problem hiding this comment.
suggestion: I'd actually personally probably set these up as ParallelVecs with one Vec for keys, one Vec for values and then probably one for the rest. It's quite possible that we end up with better perf iterating through a vector of keys than hopping through a HashMap linked list. This especially so since the iteration will likely be aggressively unrolled and SIMD'd.
And the reason for separating keys from values is that usually we're looking for a single item by key: If we iterate key-value pairs then all but once the value bytes are entirely wasted and we waste cache line space. Only once do we actually hit the right key and we know where we hit it by our iteration index.
There was a problem hiding this comment.
Nice suggestion. I had added the &'static str to avoid lifetimes while copying the spec entities but this would be better. Will work on implementing this.
There was a problem hiding this comment.
The ParallelVecs in question should be sorted, and all code should appropriately use logarithmic lookups (stdlib has us covered here with slice methods).
If we're to do away with HashMaps entirely for some parts of the code*, we should have a generic type built on ParallelVec for reuse.
* There are other hash map types and algorithms and allocator configurations? Maybe those would be more suitible? A HashMap in an arena allocator for example. I believe there are already libraries for this.
There was a problem hiding this comment.
| declarative_record: Rc<RefCell<DeclarativeEnvironment>>, | ||
|
|
||
| /// [[VarNames]] | ||
| var_names: HashMap<&'static str, ()>, |
There was a problem hiding this comment.
suggestion: Use a vector.
| /// %Array% | ||
| pub fn array() -> Object { | ||
| Object::new(Value::Object(Handle::new( | ||
| BuiltinObjectIndexes::ArrayConstructorIndex as u32, |
There was a problem hiding this comment.
This is incorrect: The constructor should be a function and the arrayconstructorindex is the index of the function's heap object data which the function heap data refers to. I had the get_constructor_index function somewhere on the heap constants file that translates the built-in object heap data index into the corresponding function index.
|
I rebased and merged this in #28 |
This is greatly inspired by Kiesel and SerenetyOS's LibJS JavaScript engines.