Skip to content

LASolver: Small refactoring#827

Merged
blishko merged 2 commits intomasterfrom
lasolver-less-bookkeeping
Mar 25, 2025
Merged

LASolver: Small refactoring#827
blishko merged 2 commits intomasterfrom
lasolver-less-bookkeeping

Conversation

@blishko
Copy link
Copy Markdown
Member

@blishko blishko commented Mar 21, 2025

Small cleanup in LASolver.

Include the removal of method updateBound.
This method was checking if the given bound (PTRef) has already been processed or not.
However, it was only called in a situation where it is guaranteed that the bound has not been seen before (the theory solver keeps track of processed inequalities with isInformed and setInformed methods in LASolver::declareAtom.

@blishko blishko requested a review from Tomaqa March 21, 2025 12:04
@blishko blishko force-pushed the lasolver-less-bookkeeping branch from 0d1b0bd to a66c52e Compare March 21, 2025 12:14
Copy link
Copy Markdown
Member

@Tomaqa Tomaqa left a comment

Choose a reason for hiding this comment

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

Looks good! I think we can also format LAVARMapper files, in a separate commit.

// Must be a new inequality
assert(tid >= LeqToLABoundRefPair.size() or (LeqToLABoundRefPair[tid] == LABoundRefPair{LABoundRef_Undef, LABoundRef_Undef}));
if (LeqToLABoundRefPair.size() <= tid) {
LeqToLABoundRefPair.growTo(tid + 1);
Copy link
Copy Markdown
Member

@Tomaqa Tomaqa Mar 24, 2025

Choose a reason for hiding this comment

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

I guess we already discussed this: Is it not performance bottleneck?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's not for OpenSMT.
I am hitting this in Golem on a few occasions, but I think I can try some workaround on the model checker level.

Note also that we tried using maps in the past (#500), but there was a few percent performance hit for OpenSMT on SMT-LIB benchmarks.

@Tomaqa Tomaqa self-requested a review March 25, 2025 21:19
Copy link
Copy Markdown
Member

@Tomaqa Tomaqa left a comment

Choose a reason for hiding this comment

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

LGTM!

blishko added 2 commits March 25, 2025 22:25
LASolver::updateBound was always called only on new inequalities, never
on already seen ones.
@blishko blishko force-pushed the lasolver-less-bookkeeping branch from a66c52e to cbde920 Compare March 25, 2025 21:26
@blishko blishko merged commit cbde920 into master Mar 25, 2025
9 checks passed
@blishko blishko deleted the lasolver-less-bookkeeping branch March 25, 2025 21:32
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