Skip to content

Add 'WeightedPSQ' and use it to sort package, flag, and stanza choices. #3594

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

Merged
merged 9 commits into from
Sep 8, 2016

Conversation

grayjay
Copy link
Collaborator

@grayjay grayjay commented Jul 22, 2016

I split this out from #2917 because the data structure is also needed for goal-scoring (#3488). WeightedPSQ associates weights with choices in the search tree. This PR uses WeightedPSQ for package, flag, and stanza choices and PSQ for goal choices, but eventually all choices should use WeightedPSQ. There are a few TODOs in the code.

/cc @kosmikus

EDIT: This PR uses [Double] as the weight type to avoid changing the behavior of the solver.

@mention-bot
Copy link

@grayjay, thanks for your PR! By analyzing the annotation information on this pull request, we identified @kosmikus, @edsko and @martinvlk to be potential reviewers

@kosmikus kosmikus self-assigned this Jul 22, 2016
@ezyang
Copy link
Contributor

ezyang commented Jul 22, 2016

Let me be the grumpy code reviewer:

  1. Needs docs!
  2. Needs asymptotics!
  3. Degree is in an odd place

But I'm all for defining data structures we need.

@kosmikus
Copy link
Contributor

@grayjay Looks promising so far. It doesn't seem to have any negative performance impact afaics.

@grayjay
Copy link
Collaborator Author

grayjay commented Jul 23, 2016

Thanks. I moved Degree into its own module, and I'll add documentation and unit tests next.

@kosmikus How do you want to proceed? Should I open a new PR with just WeightedPSQ so that it is easier to merge, or should I continue cleaning up all of these changes? I realized that I can remove some of the TODOs because they only relate to install plan scoring.

@grayjay
Copy link
Collaborator Author

grayjay commented Jul 25, 2016

I cleaned it up, added tests, and added documentation with asymptotics, similar to D.Compat.Graph. It's ready for review. I'd like to clean up the history before it is merged, though.

filter :: (v -> Bool) -> WeightedPSQ k w v -> WeightedPSQ k w v
filter p (WeightedPSQ xs) = WeightedPSQ (L.filter (p . triple_3) xs)

-- | /O(N)/.
Copy link
Contributor

@ezyang ezyang Jul 25, 2016

Choose a reason for hiding this comment

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

Ouch! Should there be a TODO here to track the length separately? (Or maybe there's a good reason not to do it eagerly related to laziness?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

length is actually no longer called. I think it was used by --reorder-goals, which now uses degree to avoid traversing the whole list. Calling length there had a significant impact on performance because it was called on the children of every available goal choice after the children were filtered with an expensive predicate. I'll remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, thanks.

@grayjay grayjay changed the title WIP: Add 'WeightedPSQ' and use it to sort package, flag, and stanza choices. Add 'WeightedPSQ' and use it to sort package, flag, and stanza choices. Jul 27, 2016
@23Skidoo
Copy link
Member

23Skidoo commented Sep 7, 2016

@grayjay @kosmikus Is this ready to go in?

@grayjay
Copy link
Collaborator Author

grayjay commented Sep 8, 2016

I was done working on it, but I wasn't sure if @kosmikus wanted to take another look.

@23Skidoo
Copy link
Member

23Skidoo commented Sep 8, 2016

Let's merge then, if @kosmikus dislikes something he can always revert.

@23Skidoo 23Skidoo merged commit df44c67 into haskell:master Sep 8, 2016
@grayjay grayjay deleted the weighted-psq branch September 8, 2016 15:11
grayjay added a commit to grayjay/cabal that referenced this pull request Sep 12, 2016
The space leak was introduced in haskell#3594. haskell#3594 added a new variable,
sortedVersions, to sort the subtrees under package choice nodes in the search
tree. However, sortedVersions referenced the subtrees and caused them to be
retained when it was not used. This commit forces evaluation of
sortedVersions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants