Skip to content

[Poach Algorithm] Term Cache#61

Draft
ajpal wants to merge 26 commits into
mainfrom
PTP_TermCache
Draft

[Poach Algorithm] Term Cache#61
ajpal wants to merge 26 commits into
mainfrom
PTP_TermCache

Conversation

@ajpal

@ajpal ajpal commented May 18, 2026

Copy link
Copy Markdown
Owner

No description provided.

@ajpal ajpal requested a review from FTRobbin May 18, 2026 21:10
@ajpal ajpal changed the title Term Cache [Poach Algorithm] Term Cache May 19, 2026
function speedup(b) {
const t = b.train.report.run_program;
const s = b.serve.wall_time_micros;
return t === 0 || s === 0 ? null : t / s;

@FTRobbin FTRobbin May 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would you mind explaining the logic here? Are we using train.report.run_program as an approximation of vanilla?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Yeah that's right. I want to measure the time for answering the query using the cache compared to the amount of time to just run the program entirely (as in vanilla egglog)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we know if this is a good approximation?

Rules: b.report.rule_micros,
Extraction: b.report.extraction_micros,
Other: b.report.other_micros,
"Program Run Time": b.train.report.run_program,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Related to the question above, we should clarify what "Program Run Time" means.

@FTRobbin FTRobbin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should add serialization size in both # terms and bytes. I see that you already have those in the reports.

Overall, this is great!

Comment thread src/poach.rs
}
}

// TODO: Add cost model from the e-graph, this is just the default (term size)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reusing the extractor's output, which includes the cost, would be better. I accept this as a workaround.

Comment thread src/poach.rs
/// Uses `Parser::default()`, which does not know about any user-defined sorts or constructors;
/// for current cache use this is fine because the cached strings are flat term shapes
/// (apps + literals). Revisit if user-defined macros start appearing in extracted terms.
fn parse_cached_term(td: &mut TermDag, s: &str) -> TermId {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There is an optimization opportunity here. Potentially clean up the code as well.

In serialization, the code transforms (TermDag, Vec<TermId>) into Vec<String>. This involves expanding each term separately, which means no sharing of common subexpressions.

In deserialization, the code transforms Vec<String> back into (TermDag, Vec<TermId>).

An optimization is to serialize and deserialize (TermDag, Vec<TermId>) directly without going through the Vec<String> representation.

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.

3 participants