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

Map Implementation #1222

Merged
merged 10 commits into from
Oct 20, 2020
Merged

Map Implementation #1222

merged 10 commits into from
Oct 20, 2020

Conversation

kustosz
Copy link
Contributor

@kustosz kustosz commented Oct 19, 2020

Pull Request Description

Implements an ordered map structure.
Implements improvements to simple thunk arguments and the case node logic.

Closes #497.

Important Notes

Small speedups accross all benchmarks. Map is 30% slower than Haskell.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

Copy link
Contributor

@iamrecursion iamrecursion left a comment

Choose a reason for hiding this comment

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

There's some stuff I'd like to discuss. Let me know when you're around to talk.

Comment on lines +61 to +62
get : Any -> Any ! No_Value_For_Key
get key =
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not want a more general Maybe a here, rather than the specific error? It seems more composable to me to use a common primitive.

Bin s k v l r ->
Bin s k (function v) (l.map function) (r.map function)
Tip -> Tip

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see a fold (left) as well.

Comment on lines 20 to 21
Helper for inserting a new key-value pair into a map.
The algorithm used here is based on the paper "Implementing Sets Efficiently
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Helper for inserting a new key-value pair into a map.
The algorithm used here is based on the paper "Implementing Sets Efficiently
Helper for inserting a new key-value pair into a map.
The algorithm used here is based on the paper "Implementing Sets Efficiently

Comment on lines 89 to 98
Controls the difference between inner and outer siblings of a heavy subtree.
Used to decide between a double and a single rotation.
ratio : Integer
ratio = 2

## PRIVATE

Controls the maximum size difference between subtrees.
delta : Integer
delta = 3
Copy link
Contributor

Choose a reason for hiding this comment

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

I would specify why these values are chosen (e.g. from the paper, from Data.Map.Strict).

@@ -0,0 +1,79 @@
from Base import all
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of where this file is. I think we should unify the collections under Base.Collection (including List and Vector). Can we talk about stdlib structure?

String languageStr = toJavaStringNode.execute(language);
String codeStr = toJavaStringNode.execute(code);

Source source = Source.newBuilder(languageStr, codeStr, "<interactive>").build();
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of the "<interactive>" here?

.getMessager()
.printMessage(
Diagnostic.Kind.ERROR,
"Argument must not be typed as Thunk. Use @Suspend instead.",
Copy link
Contributor

Choose a reason for hiding this comment

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

This error is a touch unclear. From above it looks like you have to go @Suspend Object foo, not just @Suspend foo.

* Checks whether {@code a} is lexicographically before {@code b}.
*
* @param a the left operand
* @param b the right operant
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @param b the right operant
* @param b the right operand

* @return whether {@code a} is before {@code b}.
*/
public static boolean lt(String a, String b) {
return a.compareTo(b) < 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

See my above comments about lexicographic ordering.

Comment on lines 30 to 33
#Bench_Utils.measure (here.sum_list_meta list) "list meta-fold" 1000 10
#Bench_Utils.measure (list.fold 0 (+)) "list fold" 1000 10
#Bench_Utils.measure (vec.fold 0 (+)) "vector fold" 1000 10
#Bench_Utils.measure (vec_decimal.fold 0 (+)) "vector decimal fold" 1000 10
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume these shouldn't be commented out?

@kustosz kustosz merged commit 207aaac into main Oct 20, 2020
@kustosz kustosz deleted the wip/mk/json branch October 20, 2020 11:47
@kustosz kustosz restored the wip/mk/json branch October 23, 2020 11:27
@iamrecursion iamrecursion deleted the wip/mk/json branch January 5, 2021 14: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.

Implement an Key-Value Map for Enso
2 participants