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

Stop using view in adaptive sort #45699

Merged
merged 5 commits into from
Jul 19, 2022

Conversation

LilithHafner
Copy link
Member

Fixes #45522

@LilithHafner
Copy link
Member Author

Local tests look good

(jl_2RvVlb) pkg> status
Status `/private/var/folders/hc/fn82kz1j5vl8w7lwd4l079y80000gn/T/jl_2RvVlb/Project.toml`
  [c2308a5c] FASTX v1.3.0

julia> using FASTX

julia> println(first(FASTQ.Reader(IOBuffer("@A\nA\n+\nA"))))
@A
A
+
A

I don't think we should include this test in test/sorting.jl.

base/sort.jl Show resolved Hide resolved
@ViralBShah
Copy link
Member

Should we try get this merged, or should we wait on the refactor?

@LilithHafner
Copy link
Member Author

Let's proceed here. The changes are pretty low risk, and I'd be happy to merge as is. I don't think bounds checking is necessary here given the conventions we have in this part of the codebase. Still, this code only runs for lengths > 30 ish, so we can add explicit bounds checking before the loop without measurable performance overhead if folks prefer it that way.

@ViralBShah
Copy link
Member

ViralBShah commented Jul 17, 2022

Yeah it would be good to bring the bounds check in and get this merged. Does that also pave the way to making sorting an stdlib?

@LilithHafner
Copy link
Member Author

LilithHafner commented Jul 17, 2022

I think it makes sense to improve quicksort before refactoring because one of the advantages it brings is that making quicksort stable simplifies policy, which will be good to either take advantage of (if it merges) or not wrongly hope for (if benchmarking or code review comes out catastrophically negative).

I also think that making sorting a stdlib should come after refactoring because part of sorting will have to stay behind for Core.Compiler.Sort.sort!, and I'd like to have sorting nicely formatted when it comes time to make that split.

This PR is orthoganal.

@LilithHafner
Copy link
Member Author

LilithHafner commented Jul 17, 2022

Not entirely orthogonal, that build error and merge conflict would not have happened if sorting was a stdlib.

@LilithHafner
Copy link
Member Author

This looks good to me. If other folks agree, I'd like to merge it so I can use _issorted to address #45222 (comment).

@ViralBShah
Copy link
Member

Would it be helpful to have a label for sorting?

@ViralBShah ViralBShah added the status:merge me PR is reviewed. Merge when all tests are passing label Jul 19, 2022
@ViralBShah
Copy link
Member

I believe CI is down for maintenance. So this probably needs to be held until CI is back online and we can run everything again.

@LilithHafner
Copy link
Member Author

LilithHafner commented Jul 19, 2022

Would it be helpful to have a label for sorting?

It would be helpful to me. I often find myself using the search term "sort". I'd be happy to make a label and tag some historical items if you think it's appropriate. We have Dates, TOML, Unicode and RNG labels which this feels somewhat comparable to.

So this probably needs to be held until CI is back online and we can run everything again.

The results from 11 hours ago seem pretty good to me. Is there a worry that they are a false positive?

@ViralBShah
Copy link
Member

Ok, maybe the bots are back online. I thought the runs were stale.

@ViralBShah ViralBShah added the domain:sorting Put things in order label Jul 19, 2022
@LilithHafner LilithHafner merged commit db570df into JuliaLang:master Jul 19, 2022
@LilithHafner LilithHafner deleted the remove-view-fix-45522 branch July 19, 2022 15:05
@LilithHafner LilithHafner removed the status:merge me PR is reviewed. Merge when all tests are passing label Jul 19, 2022
ffucci pushed a commit to ffucci/julia that referenced this pull request Aug 11, 2022
pcjentsch pushed a commit to pcjentsch/julia that referenced this pull request Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:sorting Put things in order
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Core.Compiler calls into sort with undefined view
3 participants