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

Use BuiltinRootNode.ArgNode to extract argument for a builtin method #12201

Merged
merged 32 commits into from
Feb 8, 2025

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jan 30, 2025

Pull Request Description

  • as Dispatch of to_text, is_a & co. on intersection types #12192 (comment) states:
  • there is a need to extract values from EnsoMultiValue in the "builtin method prelude code"
  • to enable specializations based on type of arguments, we need to wrap such prelude by a Node
  • hence introducing ArgNode & co.
  • it kinda replaces the "static code emitted" by annotation processor
  • with Truffle @Specializations done inside of ArgNode
  • making this all more Truffle-like

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 30, 2025
@JaroslavTulach JaroslavTulach self-assigned this Jan 30, 2025
@JaroslavTulach JaroslavTulach marked this pull request as draft January 30, 2025 19:57
@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Jan 30, 2025
public class Boolean extends Builtin {
public final class Boolean extends Builtin {
public Boolean() {
super(java.lang.Boolean.class);
Copy link
Member Author

Choose a reason for hiding this comment

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

Now we attempt to provide a representationType to each Builtin. That way we can deduce from signature methods having argument like boolean that the desired builtin is Boolean and the Enso type should be Standard.Base.Data.Boolean.Boolean. With that information we can extract (via CastToNode) such a type from EnsoMultiValue....

@JaroslavTulach
Copy link
Member Author

Right now there are two dataflow errors:

sbt:enso> runEngineDistribution --run test/Base_Tests/src/Semantic/Warnings_Spec.enso should.be.allowed.in.Vector|should.be.preserved.after.operations.on.Vector

[FAILED] Dataflow Warnings: [0/2, 770ms]

    - [FAILED] should be allowed in Vector [522ms]
        Reason: ['d', 'b', 'a'] did not equal [(Map_Error.Error 1 'b'), (Map_Error.Error 0 'a'), 'd']; first difference at index 0  (at Base_Tests/src/Semantic/Warnings_Spec.enso:272:9-126).


    - [FAILED] should be preserved after operations on Vector [248ms]
        Reason: ['warn4', 'warn3', 'warn2', 'warn1'] did not equal [(Map_Error.Error 3 'warn4'), (Map_Error.Error 2 'warn3'), (Map_Error.Error 1 'warn2'), (Map_Error.Error 0 'warn1')]; first difference at index 0  (at Base_Tests/src/Semantic/Warnings_Spec.enso:284:9-191).

I was debugging thru it, but I don't think I get any idea what can be the problem? Maybe something with wrapping errors? But why?

@JaroslavTulach
Copy link
Member Author

Currently there are two failures:

Again, I am not really sure why at present.

@@ -3,4 +3,8 @@
import org.enso.interpreter.dsl.BuiltinType;

@BuiltinType
public class Polyglot extends Builtin {}
public final class Polyglot extends Builtin {
public Polyglot() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd say we have way too many Builtins. Not sure why Polyglot or Debug should be builtin types at all!?

}
if (is(REQUIRES_CAST)) {
var ctx = EnsoContext.get(this);
if (this.ensoType == null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The first time ArgNode is requested to "cast", it searches for the ensoType representing the value of the argument. It would be better to do this when the ArgNode is being created, but it may be too early (before Standard.Base types are loaded in), so I haven't even tried.

Opinions?

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Jan 31, 2025

I believe the code is in shape to receive some inception review.

@JaroslavTulach JaroslavTulach marked this pull request as ready for review January 31, 2025 08:46
Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

Generally looks like a good idea. Before merging, please don't forget to benchmark it. I am interested what will be the impact on performance.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Feb 5, 2025

Checking benchmark results:

  • StringBenchmarks_lengthOfStrings - more than 15x slower
  • MultiValueBenchmarks - more than 10x slower
  • AtomBenchmarks_benchGenerateList is 4x slower
  • AtomBenchmarks_benchGenerateListQualified is 4x slower
  • ListBenchmarks_mapAnyOverList by 15%
  • EqualsBenchmarks_equalsStrings by 10%
  • EqualsBenchmarks_equalsPrimitives by less then 10%
  • ListBenchmarks_mapOverList by less then 10%

I had to patch the tool to download new results:

diff --git tools/performance/engine-benchmarks/bench_tool/__init__.py tools/performance/engine-benchmarks/bench_tool/__init__.py
index 918676fd62..11b88f0894 100644
--- tools/performance/engine-benchmarks/bench_tool/__init__.py
+++ tools/performance/engine-benchmarks/bench_tool/__init__.py
@@ -58,9 +58,9 @@ class Source(Enum):
 
     def artifact_names(self) -> List[str]:
         if self == Source.ENGINE:
-            return ["Runtime Benchmark Report"]
+            return ["Runtime Benchmark Report", "benchmark-results.xml"]
         elif self == Source.STDLIB:
-            return ["Enso JMH Benchmark Report"]
+            return ["Enso JMH Benchmark Report", "benchmark-results.xml"]
         else:
             raise ValueError(f"Unknown source {self}")

thus

And also

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Feb 5, 2025

There are overall improvements in the engine benchmarks:

AtomBenchmarks.benchGenerateList

ListBenchmarks may need some more work as this result isn't yet as great as it was:

  • ListBenchmarks_mapOverList
  • but the slowdown is less then 10%

ListBenchmarks

Update on Feb 7, 2025: Engine benchmarks seem to be fine.

mergify bot pushed a commit that referenced this pull request Feb 5, 2025
`./bench_download.py` tool stopped working after #12226. Because the artifacts on benchmark workflow runs were renamed. This PR just renames the artifacts (suggested in #12201 (comment)) and also adds some unit tests.

In many unit tests, I had to bump the date of the fetched data, because GH seems to delete workflow runs that are older than 2 years.

Note that yesterday, [Benchmark Upload](https://github.com/enso-org/enso/actions/workflows/bench-upload.yml) workflow started printing a [warning that there is an unknown artifact name](https://github.com/enso-org/enso/actions/runs/13152367074/job/36701982751#step:6:1116)
@JaroslavTulach JaroslavTulach force-pushed the wip/jtulach/BuiltinArgNode11827 branch from 83994b0 to 0a3ec13 Compare February 7, 2025 06:21
@JaroslavTulach JaroslavTulach linked an issue Feb 7, 2025 that may be closed by this pull request
@JaroslavTulach JaroslavTulach added CI: Ready to merge This PR is eligible for automatic merge CI: Keep up to date Automatically update this PR to the latest develop. labels Feb 8, 2025
@mergify mergify bot merged commit 68ccb3d into develop Feb 8, 2025
47 of 49 checks passed
@mergify mergify bot deleted the wip/jtulach/BuiltinArgNode11827 branch February 8, 2025 07:07
@unfurl-links unfurl-links bot mentioned this pull request Feb 11, 2025
5 tasks
throw sentinel;
}
if (warnings != null) {
if (warnings.hasWarnings(value)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Using hasWarnings here instead of simple warnings instanceof WithWarnings is causing problems:

Let's see what #12258 can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: Keep up to date Automatically update this PR to the latest develop. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let EnsoMultiValue.to_text delegate to first type to_text
4 participants