Some fixes to Optimisation

[Kieranoski702]

The clean/dirty optimisation implementation had a few bugs in it that were stopping it from properly marking nodes as dirty and clean when they were supposed to. Those issues have largely been fixed in this PR. There is still a few issues that I cannot think of an explanation for.

1. The number of rule application attempts for the optimized version of 1 disjunct between xyz problems is higher than without optimization (1352 vs 1222)

2. The number of rule application attempts for the optimized versions of the other disjunct amounts is lower as expected however, it still grows higher than I would expect. For example, I would expect 2 disjunct to have around double the amount of rule applications but around 10x (10114 attempts)

3. The run time for the optimized versions is around 3x that of the unoptimized versions which seems incorrect

I think one issue that may be causing this is this last part of the rewrite iteration:

// If all children are clean, mark this expression as clean
    if apply_optimizations {
        assert!(expression.children().iter().all(|c| c.is_clean()));
        expression.set_clean(true);
        return Some(Reduction::pure(expression));
    }
    None

This occurs when an expressions children are all clean. As you can see this will return a reduction with a clean expression when optimisation is enabled instead of just none. The main entry point for the rewriter, rewrite_modal looks like this:

pub fn rewrite_model<'a>(
    model: &Model,
    rule_sets: &Vec<&'a RuleSet<'a>>,
) -> Result<Model, RewriteError> {
    let rule_priorities = get_rule_priorities(rule_sets)?;
    let rules = get_rules_vec(&rule_priorities);
    let mut new_model = model.clone();
    let mut stats = RewriterStats {
        is_optimization_enabled: Some(!optimizations_disabled()),
        rewriter_run_time: None,
        rewriter_rule_application_attempts: Some(0),
        rewriter_rule_applications: Some(0),
    };

    // Check if optimizations are disabled
    let apply_optimizations = !optimizations_disabled();

    let start = std::time::Instant::now();

    while let Some(step) = rewrite_iteration(
        &new_model.constraints,
        &new_model,
        &rules,
        apply_optimizations,
        &mut stats,
    ) {
        step.apply(&mut new_model); // Apply side-effects (e.g. symbol table updates)
    }
    stats.rewriter_run_time = Some(start.elapsed());
    model.context.write().unwrap().stats.add_rewriter_run(stats);
    Ok(new_model)
}

The while loop here will keep trying to apply until the full tree returns none. I think something is going wrong with these two facts. For example, if I change the rewrite_iteration ending to look like this:

// If all children are clean, mark this expression as clean
assert!(expression.children().iter().all(|c| c.is_clean()));
expression.set_clean(true);
Some(Reduction::pure(expression));

Then the new test I added runs forever (or an extremely long time) which suggests that None has to be returned and we know that the unoptimized version will do this faster as it never returns a reduction to set itself to clean.

I am not sure if this is the issue but if it is I don't really know how to fix this so any help on this would be appreciated! @ozgurakgun mentioning you as you said you wanted to see the PR when it was up!

[ozgurakgun]

unrelated to this PR, but the documentation/code coverage action seems broken after we merged the latest changes @PedroGGBM

[ozgurakgun]

Hi @Kieranoski702 - when you find time for this, can you please update the PR and turn off the optimisation by default?

I want to merge this as is and work on it after.

Thanks! No rush!

[Kieranoski702]

@ozgurakgun I've sorted all the merge conflicts, turned off optimisations by default and updated the expected results for the tests accordingly. Seems to work all fine on my (ubuntu) machine and the CI passes but might be worth checking on your own system as well. Other than that though this should be ready to merge!

[ChrisJefferson]

Not a requirement for this PR, but just information -- if there are some fields which you don't want saving in the output, serde has a skip option, which lets you set variables to a default value -- I don't know if the 'metadata' always wants saving in the output?

[ozgurakgun]

I think we want it for the tests, at least for now. When we think we got the optimisation right we might replace their printing with an assertion.

[ozgurakgun]

Maybe more importantly, clean/dirty will need to correspond to rule levels instead of being a bool. More on this soon...