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.

Sign up for GitHub

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

Jump to bottom

Convert Any.== to a builtin #3956

Merged
merged 43 commits into from
Dec 29, 2022
Merged

Convert Any.== to a builtin #3956

merged 43 commits into from
Dec 29, 2022

Conversation

Akirathan
Copy link
Member

@Akirathan Akirathan commented Dec 7, 2022 •
edited
Loading

Convert Any.== to a builtin

Pull Request Description

Any.== is a builtin method. The semantics is the same as it used to be, except that we no longer assume x == y iff Meta.is_same_object x y, which used to be the case and caused failures in table tests.

Important Notes

Measurements from EqualsBenchmarks shows that the performance of Any.== for recursive atoms increased by roughly 20%, and the performance for primitive types stays roughly the same.

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 code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@Akirathan Akirathan self-assigned this Dec 14, 2022
@Akirathan Akirathan marked this pull request as ready for review December 16, 2022 11:56
JaroslavTulach
JaroslavTulach requested changes Dec 20, 2022
View reviewed changes
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

You cannot use transferToInterpreterAndInvalidate in my opinion. Use @TruffleBoundary.

distribution/lib/Standard/Base/0.0.0-dev/src/Any.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Time/Date_Time.enso Show resolved Hide resolved
.../runtime/src/bench/java/org/enso/interpreter/bench/benchmarks/semantic/EqualsBenchmarks.java Show resolved Hide resolved
test/Tests/src/Semantic/Equals_Spec.enso Show resolved Hide resolved
test/Tests/src/Semantic/Equals_Spec.enso Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsNode.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsNode.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsNode.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsNode.java Outdated Show resolved Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/EqualsNode.java Outdated Show resolved Hide resolved
@Akirathan Akirathan mentioned this pull request Dec 23, 2022
Merged
4 tasks
Akirathan added a commit that referenced this pull request Dec 23, 2022
@Akirathan
[WIP] QuickFix for HashCodeTest before PR #3956 is merged
9160e91
radeusgd
radeusgd approved these changes Dec 29, 2022
View reviewed changes
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks good now! Thanks for going through all of this back-and-forth.

@Akirathan Akirathan force-pushed the wip/akirathan/any-equal-183338334 branch from f4a9f89 to 65d979f Compare December 29, 2022 16:10
@Akirathan Akirathan added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Dec 29, 2022
@mergify mergify bot merged commit e6838bc into develop Dec 29, 2022
@mergify mergify bot deleted the wip/akirathan/any-equal-183338334 branch December 29, 2022 21:20
Akirathan added a commit that referenced this pull request Jan 19, 2023
@Akirathan
All Enso objects are hasheable (#3878)
fcc2163 
* Hash codes prototype

* Remove Any.hash_code

* Improve caching of hashcode in atoms

* [WIP] Add Hash_Map type

* Implement Any.hash_code builtin for primitives and vectors

* Add some values to ValuesGenerator

* Fix example docs on Time_Zone.new

* [WIP] QuickFix for HashCodeTest before PR #3956 is merged

* Fix hash code contract in HashCodeTest

* Add times and dates values to HashCodeTest

* Fix docs

* Remove hashCodeForMetaInterop specialization

* Introduce snapshoting of HashMapBuilder

* Add unit tests for EnsoHashMap

* Remove duplicate test in Map_Spec.enso

* Hash_Map.to_vector caches result

* Hash_Map_Spec is a copy of Map_Spec

* Implement some methods in Hash_Map

* Add equalsHashMaps specialization to EqualsAnyNode

* get and insert operations are able to work with polyglot values

* Implement rest of Hash_Map API

* Add test that inserts elements with keys with same hash code

* EnsoHashMap.toDisplayString use builder storage directly

* Add separate specialization for host objects in EqualsAnyNode

* Fix specialization for host objects in EqualsAnyNode

* Add polyglot hash map tests

* EconomicMap keeps reference to EqualsNode and HashCodeNode.

Rather than passing these nodes to `get` and `insert` methods.

* HashMapTest run in polyglot context

* Fix containsKey index handling in snapshots

* Remove snapshots field from EnsoHashMapBuilder

* Prepare polyglot hash map handling.

- Hash_Map builtin methods are separate nodes

* Some bug fixes

* Remove ForeignMapWrapper.

We would have to wrap foreign maps in assignments for this to be efficient.

* Improve performance of Hash_Map.get_builtin

Also, if_nothing parameter is suspended

* Remove to_flat_vector.

Interop API requires nested vector (our previous to_vector implementation). Seems that I have misunderstood the docs  the first time I read it.

- to_vector does not sort the vector by keys by default

* Fix polyglot hash maps method dispatch

* Add tests that effectively test hash code implementation.

Via hash map that behaves like a hash set.

* Remove Hashcode_Spec

* Add some polyglot tests

* Add Text.== tests for NFD normalization

* Fix NFD normalization bug in Text.java

* Improve performance of EqualsAnyNode.equalsTexts specialization

* Properly compute hash code for Atom and cache it

* Fix Text specialization in HashCodeAnyNode

* Add Hash_Map_Spec as part of all tests

* Remove HashMapTest.java

Providing all the infrastructure for all the needed Truffle nodes is no longer manageable.

* Remove rest of identityHashCode message implementations

* Replace old Map with Hash_Map

* Add some docs

* Add TruffleBoundaries

* Formatting

* Fix some tests to accept unsorted vector from Map.to_vector

* Delete Map.first and Map.last methods

* Add specialization for big integer hash

* Introduce proper HashCodeTest and EqualsTest.

- Use jUnit theories.
- Call nodes directly

* Fix some specializations for primitives in HashCodeAnyNode

* Fix host object specialization

* Remove Any.hash_code

* Fix import in Map.enso

* Update changelog

* Reformat

* Add truffle boundary to BigInteger.hashCode

* Fix performance of HashCodeTest - initialize DataPoints just once

* Fix MetaIsATest

* Fix ValuesGenerator.textual - Java's char is not Text

* Fix indent in Map_Spec.enso

* Add maps to datapoints in HashCodeTest

* Add specialization for maps in HashCodeAnyNode

* Add multiLevelAtoms to ValuesGenerator

* Provide a workaround for non-linear key inserts

* Fix specializations for double and BigInteger

* Cosmetics

* Add truffle boundaries

* Add allowInlining=true to some truffle boundaries.

Increases performance a lot.

* Increase the size of vectors, and warmup time for Vector.Distinct benchmark

* Various small performance fixes.

* Fix Geo_Spec tests to accept unsorted Map.to_vector

* Implement Map.remove

* FIx Visualization tests to accept unsorted Map.to_vector

* Treat java.util.Properties as Map

* Add truffle boundaries

* Invoke polyglot methods on java.util.Properties

* Ignore python tests if python lang is missing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radeusgd radeusgd radeusgd approved these changes

@JaroslavTulach JaroslavTulach JaroslavTulach approved these changes

@jdunkerley jdunkerley Awaiting requested review from jdunkerley jdunkerley is a code owner

@4e6 4e6 Awaiting requested review from 4e6 4e6 is a code owner

@hubertp hubertp Awaiting requested review from hubertp hubertp is a code owner

Assignees

@Akirathan Akirathan

Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Akirathan @radeusgd @hubertp @jdunkerley @JaroslavTulach