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

Removing Unsafe.set_atom_field #4023

Merged
merged 27 commits into from
Jan 9, 2023
Merged

Removing Unsafe.set_atom_field #4023

merged 27 commits into from
Jan 9, 2023

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jan 4, 2023 •
edited
Loading

Pull Request Description

Introducing Meta.atom_with_hole to create an Atom with a hole that is then safely filled in later.

Important Notes

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.

@JaroslavTulach JaroslavTulach self-assigned this Jan 4, 2023
@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jan 5, 2023
kustosz
kustosz reviewed Jan 5, 2023
View reviewed changes
Copy link
Contributor

@kustosz kustosz left a comment

Choose a reason for hiding this comment

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

I have some doubts regarding the user facing API and also the internals, though from the high level I like this solution. I also like it's all self-contained, not polluting what Atoms really are. Also, do you have any info on the performance impact of this?

distribution/lib/Standard/Base/0.0.0-dev/src/Data/List.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso Outdated Show resolved Hide resolved
...me/src/main/java/org/enso/interpreter/node/expression/builtin/meta/LazyAtomInstanceNode.java Outdated Show resolved Hide resolved
...me/src/main/java/org/enso/interpreter/node/expression/builtin/meta/LazyAtomInstanceNode.java Outdated Show resolved Hide resolved
@JaroslavTulach
Copy link
Member Author

Also, do you have any info on the performance impact of this?

Here is a benchmark measuring List.map - 695775b - currently it is not fast and I need to check IGV to find out what can be improved.

jdunkerley
jdunkerley approved these changes Jan 6, 2023
View reviewed changes
Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

very minor points but lgtm

distribution/lib/Standard/Base/0.0.0-dev/src/Data/List.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Meta.enso Outdated Show resolved Hide resolved
@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/NoSetAtomField_183578531 branch from fea0260 to 8ccbecb Compare January 6, 2023 15:13
@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Jan 6, 2023
@JaroslavTulach
Copy link
Member Author

I think we are ready to integrate. The only problem is 2-3x slowdown - but I'd like to resolve it later. Right now I feel there are other important issues to focus on.

@mergify mergify bot merged commit 41b2aac into develop Jan 9, 2023
@mergify mergify bot deleted the wip/jtulach/NoSetAtomField_183578531 branch January 9, 2023 13:39
hubertp
hubertp reviewed Jan 9, 2023
View reviewed changes
Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

Minor nits

project/FrgaalJavaCompiler.scala
Comment on lines +140 to +142
if (in.isDefined) {
ensoProperties.setProperty("input", in.get.toString())
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

      in.foreach { prop => 
        ensoProperties.setProperty("input", prop.toString())
      }

...ntime/src/main/java/org/enso/interpreter/node/expression/builtin/meta/AtomWithAHoleNode.java
} else {
if (lastIndex != -2) {
CompilerDirectives.transferToInterpreterAndInvalidate();
lastIndex = -2;
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice if those magic constants -1 and -2 were documented somehow.

JaroslavTulach added a commit that referenced this pull request Jan 10, 2023
@JaroslavTulach
Keep the original format of List.to_text corrupted by #4023
9b393d4
@JaroslavTulach JaroslavTulach mentioned this pull request Jan 10, 2023
Merged
3 tasks
@wdanilo wdanilo mentioned this pull request Feb 6, 2023
Closed
@JaroslavTulach JaroslavTulach mentioned this pull request Sep 12, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kustosz kustosz kustosz left review comments

@hubertp hubertp hubertp left review comments

@jdunkerley jdunkerley jdunkerley approved these changes

@4e6 4e6 4e6 approved these changes

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

Assignees

@JaroslavTulach JaroslavTulach

Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. 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
@JaroslavTulach @hubertp @4e6 @kustosz @jdunkerley