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

Feature/inc cache propagation #1220

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

dpl0a
Copy link
Contributor

@dpl0a dpl0a commented Apr 3, 2023

This PR implements part of the IncCache interface for incremental merging and modifies some functions related to merging records.

@dpl0a dpl0a force-pushed the feature/IncCache-propagation branch from df97144 to 152993b Compare April 3, 2023 15:22
@dpl0a dpl0a force-pushed the feature/IncCache-propagation branch from 152993b to 17031c9 Compare April 3, 2023 15:38
@github-actions github-actions bot temporarily deployed to pull request April 3, 2023 15:42 Inactive
@github-actions github-actions bot temporarily deployed to pull request April 3, 2023 16:19 Inactive
@github-actions github-actions bot temporarily deployed to pull request April 3, 2023 16:55 Inactive
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in for merging this soon and not letting this PR stale, however, it adds non trivial changes, including in the main codebase. I think it really needs more documentation and comments, if we want to be able to understand what's going on a few months from now.

Beside, I wonder if we might also update Thunk::saturate to work "in-place", now that we revert everything in advance anyway. But that's not very important for this PR, I'm just thinking out loud.

Comment on lines +100 to +102
fn smart_clone(&mut self, v: Vec<CacheIndex>) -> HashMap<CacheIndex, CacheIndex>;

fn propagate_dirty(&mut self, indices: Vec<CacheIndex>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to document those functions, even just a draft, waiting for more. Those are the cornerstones of incremental evaluation, and are now used inside merge.

@@ -370,4 +314,67 @@ impl Cache for IncCache {
RichTerm::from(Term::App(partial_app, arg))
})
}

fn smart_clone(&mut self, v: Vec<CacheIndex>) -> HashMap<CacheIndex, CacheIndex> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this functions need comments inside

@yannham
Copy link
Member

yannham commented Apr 4, 2023

I ran the benchmarks locally, and changes are within the noise threshold (not in the sense of criterion, which shows +- 5 to 10% depending on the benchmarks, but that's unfortunately something common, which is also observed across run of the same benchmarks on the same branch). The mantis benchmark seemed to take a small hit, but the numbers are volatile enough so that it doesn't seem to be a huge effect.

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

Successfully merging this pull request may close these issues.

2 participants