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

refactor PagedMergeSort #1

Merged

Conversation

LilithHafner
Copy link

I feel like these suggestions are a bit to comprehensive to put in github line-by-line-comments so I made a PR to your topic branch, @LSchwerdt.

I suggest these substantial changes:

  • Remove the 1,2,3 from the beginning of the pageLocation array and move the final partial page directly into position, removing it from pageLocation as well, shortening the array by a total of 4 and helping simplify code.
  • Shift responsibility to copy data back from the buffer from copy_pages! to the caller which allows that function to be substantially simpler
  • Refactor how the merge is handled once one of A or B is close to empty theoretically resulting in minor (negligible) performance improvement and also simplifying that code and setting up for "calculate location of the 3 free pages" to be much simpler
  • Pretty much eliminate the "calculate location of the 3 free pages" section altogether
  • Switch from a while loop to a slightly more concise for loop when doing the final unresolved cycle detection pass. Also skip the first 4 blocks because the first three are already in place and any nontrivial cycle will appear in at least 2 positions so skipping one is okay.
  • remove copy_page!(v, source, offset, offset2, pagesize) and replace its usages with copyto! which is equivalent for Vectors but may have more efficient implementations for other AbstractVectors

And these more superficial changes:

  • inline assignments of the form
x = 1
f(x)
  • inline internal functions next_page_A and next_page_B that are only called once
  • replace unused values in unpacking with _

@LSchwerdt
Copy link
Owner

Amazing work! Sorry for taking so long for my comments. But I only have some minor remarks.

src/SortingAlgorithms.jl Outdated Show resolved Hide resolved
src/SortingAlgorithms.jl Outdated Show resolved Hide resolved
src/SortingAlgorithms.jl Outdated Show resolved Hide resolved
@LSchwerdt LSchwerdt merged commit bfbe0cf into LSchwerdt:pagedmergesort May 28, 2023
@LilithHafner LilithHafner deleted the pagedmergesort-lilith-refactor branch May 28, 2023 14:33
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