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

feat: Optimize sort by Below #1457

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: Optimize sort by Below #1457

wants to merge 6 commits into from

Conversation

seebees
Copy link
Contributor

@seebees seebees commented Oct 22, 2024

The Below function is in the hot path.
The slice (x[1..]) operation is not optimized in Dafny. This optimizes this function by turning the recursive slice into a loop over an index into the seq.
Further, a bounded integer version is also included.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@seebees seebees requested a review from a team as a code owner October 22, 2024 19:55
@texastony
Copy link
Contributor

Dafny Verifier crashed on 4dfd05d:

2024-10-22T20:11:35.9801750Z Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
2024-10-22T20:11:35.9811410Z    at Microsoft.Boogie.ReadOnlyVisitor.Visit(Microsoft.Boogie.Absy)
2024-10-22T20:11:35.9824210Z    at Microsoft.Boogie.ReadOnlyVisitor.VisitDeclarationList(System.Collections.Generic.List`1<Microsoft.Boogie.Declaration>)
2024-10-22T20:11:35.9828510Z    at Microsoft.Boogie.ReadOnlyVisitor.VisitProgram(Microsoft.Boogie.Program)
2024-10-22T20:11:35.9832780Z    at Microsoft.Boogie.BasicTypeVisitor..ctor(Microsoft.Boogie.Absy)
2024-10-22T20:11:35.9843010Z    at Microsoft.Boogie.Checker.Target(Microsoft.Boogie.Program, Microsoft.Boogie.ProverContext, VC.Split)
2024-10-22T20:11:35.9846800Z    at VC.CheckerPool.PrepareChecker(Microsoft.Boogie.Program, VC.Split, Microsoft.Boogie.Checker)
2024-10-22T20:11:35.9849590Z    at VC.CheckerPool+<FindCheckerFor>d__7.MoveNext()
2024-10-22T20:11:35.9852360Z    at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(System.Threading.Thread, System.Threading.ExecutionContext, System.Threading.ContextCallback, System.Object)
2024-10-22T20:11:35.9861690Z    at System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1+AsyncStateMachineBox`1[[System.__Canon, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e],[VC.CheckerPool+<FindCheckerFor>d__7, Boogie.VCGeneration, Version=3.2.3.0, Culture=neutral, PublicKeyToken=null]].MoveNext(System.Threading.Thread)
2024-10-22T20:11:35.9877740Z    at System.Threading.ThreadPoolWorkQueue.Dispatch()
2024-10-22T20:11:35.9880970Z    at System.Threading.PortableThreadPool+WorkerThread.WorkerThreadStart()
2024-10-22T20:11:35.9885100Z    at System.Threading.Thread.StartCallback()
2024-10-22T20:11:36.1982370Z make: *** [verify_service] Abort trap: 6
2024-10-22T20:11:36.3739850Z ##[error]Process completed with exit code 2.

@@ -148,11 +148,89 @@ module SortCanon {
}
}

predicate method Below(x: seq<uint8>, y: seq<uint8>) {
predicate Below(x: seq<uint8>, y: seq<uint8>) {
Copy link
Contributor

@texastony texastony Oct 23, 2024

Choose a reason for hiding this comment

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

Forgive me, but I cannot figure out, based on the context available, what the contract of this method is.

What are the requirements for a seq<uint8> to be below another seq<uint8>?

If |x| == 0 and |y| != 0, is x below y?
x is certainly shorter.

But the sum of a thousand zeros is than one one.
So... in that case,
if y is a 1k zeros, and x is 1 one,
is x really below y?

I take it the heuristic is not summation,
but some other criteria.

But I cannot figure out what that criteria is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a kind of ordering.

assert Below([1,2,3], [1,2,4])
assert !Below([1,2,3], [1,2])

Copy link
Contributor

Choose a reason for hiding this comment

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

So... is [1] below [0,0,0]?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or does it not matter, so long as the ordering is consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, the ordering being consistent is key. I think that it would be assert Below([1], [0,0,0]).

This is like lex ordering since !(1 < 0)

The `Below` function is in the hot path.
The slice (x[1..]) operation is not optimized in Dafny.
This optimizes this function by turning the recursive slice
into a loop over an index into the seq.
Further, a bounded integer version is also included.
The `MergeSort` function is in the hot path.
The slice (x[1..]) operation is not optimized in Dafny.
This optimizes this function by turning the recursive slice
into a loop over an index into the seq.
Further, a bounded integer version is also included.

It also limits the total amount of data copied.
@seebees seebees force-pushed the seebees/optimize-below branch from 7dc718c to ea794cc Compare November 1, 2024 20:06
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