CarterFendley / fast-dep

MIT License
1 stars 1 forks source link

Take a look at the `DepNode` ownership pattern #5

Open CarterFendley opened 2 months ago

CarterFendley commented 2 months ago

As noted in review with Jonathan, we are doing a .clone() on ever time where we create a DepNode https://github.com/CarterFendley/fast-dep/blob/d4ef6f517a9a3e9225d8f5ed4bc64c89d824e99f/src/core/builder.rs#L327. This is due to the ModuleSpec struct being moved into _process_imports(...).

Potentially if _process_imports(...) could be modified to not require an owned copy of ModuleSpec, but only operate on a reference this could be solved. Additionally, currently we are transferring DepNode to DepGraph as soon as it is created, maybe this makes pulling out a reference a bit more complicated? Not sure.

SchwartzCode commented 2 months ago

You probably need to either introduce lifetimes to the struct or use some form of Rc to get around the cloning. Another option is to have the nodes stored elsewhere, and just an index to the node in that structure is passed. Kind of sketchy, but we've done some of that for search problems at work to speed things up

Note: I don't think this lifetime approach works if spec needs to be mutated elsewhere after it is passed to this struct. Also probably worth using string references to cut down on copying

pub struct DepNode <'a> {
    #[pyo3(get)]
    pub name: String,
    #[pyo3(get)]
    pub spec: &'a importlib::ModuleSpec,
    // The dependencies & dependents by spec.name
    dependencies: HashSet<&'a String>,
    #[pyo3(get)]
    dependents: HashSet<&'a String>,
    #[pyo3(get)]
    depth: Option<i32>
}