Skip to content

Refactor dstep and enable benchmark#25

Open
ninioArtillero wants to merge 4 commits into
seereason:masterfrom
tweag:ninioArtillero/bench-refactor
Open

Refactor dstep and enable benchmark#25
ninioArtillero wants to merge 4 commits into
seereason:masterfrom
tweag:ninioArtillero/bench-refactor

Conversation

@ninioArtillero
Copy link
Copy Markdown
Contributor

@ninioArtillero ninioArtillero commented May 13, 2026

This PR introduces a refactor that changes dstep to use a single list traversal. Testing the change with (the now enabled) cabal bench I observed a ~1ms (10 %) improvement locally on average. In my opinion, this refactor improves the conciseness and clarity of this code. The test suite (cabal test) passes without casualties.

Note this introduces no API or behavior changes.

The existing bench/bench.hs file was added to a benchmark component and tweaked just to succesfully compile and run with cabal bench. This files contains a single benchmark running the base diff algorithm (getDiff) on two 1,000 random Bools lists.

The change

Previously, the local nextDLs traversed the list producing two new nodes for each element, one produced by a horizontal step (dl) and the other by a vertical step (dl').

        nextDLs [] = []
        nextDLs (dl:rest) = dl':dl'':nextDLs rest
          where dl'  = addsnake cd $ dl {poi=poi dl + 1, path=(F : pdl)}
                dl'' = addsnake cd $ dl {poj=poj dl + 1, path=(S : pdl)}
                pdl = path dl

Now, nextDLs has been replaced by two helpers that locally expose those inner operations: hStep and vStep.

    hStep node = addsnake cd $ node {poi = poi node + 1, path = F : path node}
    vStep node = addsnake cd $ node {poj = poj node + 1, path = S : path node}

Previously, the detailed behavior of selectBestDLFromPairs was not evident; it depended on the specific arrangement of elements passed to it nextDLs.

dstep cd dls = hd:selectBestDLFromPairs rst
  where (hd:rst) = nextDLs dls

Now, with the new stepAndMerge definition it is easier to track how the precondition of furthestReaching is fulfilled: matching k-diagonal candidates are produced from successive input nodes and merged at each recursive call:

-- The resulting candidates are merged pairwise: the vertical successor of each
-- node is paired with the horizontal successor of the next node in the wave
-- front.

...

    stepAndMerge :: DL -> [DL] -> [DL]
    stepAndMerge prev [] = [vStep prev]
    stepAndMerge prev (next:rest) =
      furthestReaching (vStep prev) (hStep next) : stepAndMerge next rest

Finally, the main body of the function is a bit more explicit about how the new wave front is constructed

dstep _ [] = error "dstep: Cannot perform expansion on an empty list of nodes"
dstep cd (dl:dls) = hStep dl : stepAndMerge dl dls

Note that now the empty list case is explicitly addressed; previously this case was implicitly neglected by the pattern match

  where (hd:rst) = nextDLs dls

Comment thread src/Data/Algorithm/Diff.hs
Comment thread Diff.cabal
Co-authored-by: Facundo Domínguez <facundominguez@gmail.com>
@ninioArtillero ninioArtillero marked this pull request as ready for review May 13, 2026 16:47
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