WithWarnings uses EnsoHashMap to speed things up #10555

Akirathan · 2024-07-15T14:04:57Z

Pull Request Description

Majority of warnings handling is now done via newly introduced nodes. Moreover, the underlying representation of warnings storage in WithWarnings was changed from Warning[] to EnsoHashMap.

Important Notes

Remove ArrayRope.

Checklist

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

Ensure no regressions in engine and stdlib benchmarks

…essenger instead

JaroslavTulach

Pavel, you wrote:

the performance is much worse so far, will investigate closely tomorrow

I think the approach you are using is correct. I cannot say why it is slow.

I'd like to advocate usage of specialized Node classes to manipulate WithWarnings. Ideally WithWarnings is an opaque class without any methods that can only be manipulated by associated nodes.

engine/runtime/src/main/java/org/enso/interpreter/node/callable/IndirectInvokeCallableNode.java

...e/runtime/src/main/java/org/enso/interpreter/node/callable/IndirectInvokeConversionNode.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/vector/Array.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/Warning.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/error/WithWarnings.java

JaroslavTulach · 2024-07-18T05:54:32Z

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/MethodProcessor.java

 out.println(" @Override");
 out.println(" public Object call(VirtualFrame frame, Object[] args) {");
- out.println(" return handleExecute(frame, extra, body, args);");
+ out.println(" return handleExecute(frame, extra, body, insertNode, interop, args);");


Interesting. I can't say I like this at all! I don't want new arguments to handleExecute, if possible... can we keep handleExecute untouched and just wrap all calls to handleExecute with proper warnings handling magic?

That sounds very difficult. handleExecutes strips warnings from almost all the arguments, and then reassembles these warnings into the result. Doing this outside of handleExecute would mean that we would need to process arguments also outside handleExecute.

Why don't you like additional arguments to handleExecute?

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/MethodProcessor.java

This reverts commit a0dc44e.

This reverts commit c4ab6b3.

This reverts commit a5dba6f.

Removed methods: - WithWarnings.wrap - WithWarnings.appendTo - WithWarnings.append

…essor

JaroslavTulach · 2024-08-06T05:05:02Z

lib/scala/interpreter-dsl/src/main/java/org/enso/interpreter/dsl/MethodProcessor.java

 out.println(
 " if ("
 + arrayRead(argumentsArray, arg.getPosition())
- + " instanceof WithWarnings) {");
+ + " instanceof WithWarnings withWarnings) {");


Recently I was claiming to @GregoryTravis that instanceof WithWarnings isn't the right way to check for warnings. Is or isn't it the right way in this case? Is or isn't it the right way in Greg's case?

In this case, checking via warnsLibrary.hasWarnings(x) is not possible. The reason is that later, we fetch the value out of withWarnings via withWarnings.getValue(), which retains the old warnings. If you checked for warnings with warnsLibrary.hasWarnings(x), you would need to fetch the value via warnsLibrary.removeWarnings(x), and that does not retain old warnings. I have already tried to implement it that way, but some tests in Warning_Spec on Warning.get_all wrap_errors=True were failing.

Is or isn't it the right way in Greg's case?

Generally, I would prefer the check to be done via WarningsLibrary. This case in MethodProcessor is a bit special, though.

On my branch, without these changes, the WarningsLibrary approach looks good:

#10725 (comment)

Should I wait for this to be merged and benchmark my changes before merging them?

@GregoryTravis I have just scheduled benchmarks for this PR. All other tests are already successful. If benchmarks are OK, I will merge it. Wait, please before I compare the benchmarks, I will ping you then.

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/hash/EnsoHashMapBuilder.java

Hash size is fetched via HashMapSizeNode.

Akirathan · 2024-08-06T16:54:40Z

Engine benchmarks scheduled at: https:/enso-org/enso/actions/runs/10270716008
Stdlib benchmarks scheduled at: https:/enso-org/enso/actions/runs/10270723661

GitHub
Benchmark Engine · enso-org/enso@a5878b2
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

GitHub
Benchmark Standard Libraries · enso-org/enso@a5878b2
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

JaroslavTulach

This is a huge change and it'd be better to get it finally in. These are the benchmarks results I see:

sbt:runtime-benchmarks> benchOnly Warning
[info] Benchmark                                          Mode  Cnt   Score   Error  Units
[info] WarningBenchmarks.diffWarningRandomElementsVecSum  avgt    3  12.885 ± 3.815  ms/op
[info] WarningBenchmarks.noWarningsVecSum                 avgt    3   0.006 ± 0.001  ms/op
[info] WarningBenchmarks.randomElementsVecSum             avgt    3   0.034 ± 0.014  ms/op
[info] WarningBenchmarks.sameWarningVecSum                avgt    3   1.842 ± 0.306  ms/op

It is far from ideal, but if we are faster than current develop and CI is green, let's integrate and continue this endeavor in another PR.

...e/runtime/src/main/java/org/enso/interpreter/node/callable/IndirectInvokeConversionNode.java

engine/runtime/src/main/java/org/enso/interpreter/node/callable/IndirectInvokeMethodNode.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/hash/EnsoHashMapBuilder.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/hash/HashMapInsertAllNode.java

Akirathan · 2024-08-07T08:49:37Z

Comparing the benchmark results with develop:

In engine benchmarks, I can see that all WarningBenchmarks results are better than on develop. More specifically:
- WarningBenchmarks.diffWarningRandomElementsVecSum: 137 vs 27
- WarningBenchmarks.noWarningsVecSum: 0.16 vs 0.0031
- WarningBenchmarks.randomElementsVecSum: 0.206 vs 0.022
- WarningBenchmarks.sameWarningVecSum: 8.311 vs 1.757
stdlib benchmarks are without any regressions.

Conclussion: Benchmarks look very good.

Refactor if instanceof to pattern-match variant

77efb09

Akirathan added CI: No changelog needed Do not require a changelog entry for this PR. CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Jul 15, 2024

Akirathan self-assigned this Jul 15, 2024

Akirathan added 4 commits July 17, 2024 18:52

WithWarnings uses EnsoHashMap to store warnings

061e9e4

MethodProcessor creates HashMapInsertNode in every builtin method

3a4d36c

Annotations processor does not throw error, but uses processing env m…

8f02245

…essenger instead

Make some cached nodes in Warning Shared

d24bfeb

JaroslavTulach reviewed Jul 18, 2024

View reviewed changes

JaroslavTulach mentioned this pull request Jul 18, 2024

Migrate WithWarnings to use EnsoHashMap to speed them up significantly #8682

Closed

Akirathan added 7 commits July 18, 2024 12:18

Refactor WarningsTest to use ContextUtils

6ff3915

Move all warning-related classes to new package

68bcbd6

WarningsLibrary is part of the type system

1c74dfe

Use new HasWarningsNode instead of WarningsLibrary.hasWarnings

a5dba6f

Remove WarningsLibrary.hasWarnings message

c4ab6b3

Fix infinite recursion in ArraySlice.readArrayElement

a0dc44e

GenerateUncached variants for ArrayLike nodes

b4d6c3f

enso-bot bot mentioned this pull request Jul 19, 2024

Up to 200% benchmarks regression after integrating Comparable.from return type check #10539

Closed

Akirathan added 12 commits July 19, 2024 10:55

Revert "Fix infinite recursion in ArraySlice.readArrayElement"

120fdd4

This reverts commit a0dc44e.

Revert "Remove WarningsLibrary.hasWarnings message"

3645231

This reverts commit c4ab6b3.

Revert "Use new HasWarningsNode instead of WarningsLibrary.hasWarnings"

9e5edc5

This reverts commit a5dba6f.

Replace warning.attach by dedicated AppendWarningNode

dd2655b

Removed methods: - WithWarnings.wrap - WithWarnings.appendTo - WithWarnings.append

Remove unused WithWarnings constructors

a2ed4e5

Remove unused methods

267a22f

Array.collectAllWarnings is not behind TruffleBoundary

5f44b67

Make sure WarningsLibrary is correctly renamed in builtin method proc…

eedbb1c

…essor

Add specializations for a single warning to AppendWarningNode

47b06ac

Warning.attach handles generic Object as value

68582bf

ArrayRope implements InteropLibrary

737c472

Warning.get_all is implemented as dedicated node

3f41e19

Akirathan added 2 commits August 5, 2024 18:18

fix typo in MethodProcessor

9d732a9

Replace CompilerAsserts.neverPartOfCompilation with TruffleBoundary

1629773

JaroslavTulach reviewed Aug 6, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/data/hash/EnsoHashMapBuilder.java Outdated Show resolved Hide resolved

Akirathan added 5 commits August 6, 2024 10:41

tests in interpreter-dsl-test enter polyglot context

d09f0ce

Methods in EnsoHashMapBuilder are package-protected

39f7262

TailCallException stores warnings as EnsoHashMap

e887db5

Make EnsoHashMap.getCachedVectorRepresentation methods package-protected

2d7a8b3

EnsoHashMap.getHashSize is package-protected.

0ecab16

Hash size is fetched via HashMapSizeNode.

Akirathan marked this pull request as ready for review August 6, 2024 09:49

Akirathan requested review from jdunkerley, radeusgd, GregoryTravis, AdRiley, marthasharkey, 4e6 and hubertp as code owners August 6, 2024 09:49

fmt

a5878b2

JaroslavTulach approved these changes Aug 7, 2024

View reviewed changes

Akirathan added 3 commits August 7, 2024 11:43

EntriesIterator is final

a44cf5a

Use EnsoHashMap for warnings instead of an array.

a88ae74

Change signature of HashMapInsertAllNode to accept only EnsoHashMaps

ac6dfb4

Akirathan mentioned this pull request Aug 7, 2024

Attaching a warning to error is no op #10765

Merged

2 tasks

GregoryTravis approved these changes Aug 7, 2024

View reviewed changes

Akirathan added the CI: Ready to merge This PR is eligible for automatic merge label Aug 7, 2024

mergify bot merged commit c2c6712 into develop Aug 7, 2024
41 of 42 checks passed

mergify bot deleted the wip/akirathan/8682-with-warnings-hashmap branch August 7, 2024 15:30

enso-bot bot mentioned this pull request Aug 7, 2024

Store FramePointer in the IR and cache it #10129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WithWarnings uses EnsoHashMap to speed things up #10555

WithWarnings uses EnsoHashMap to speed things up #10555

Akirathan commented Jul 15, 2024 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Jul 18, 2024

Akirathan Aug 5, 2024

JaroslavTulach Aug 6, 2024

Akirathan Aug 6, 2024

Akirathan Aug 6, 2024

GregoryTravis Aug 6, 2024

Akirathan Aug 6, 2024

Akirathan commented Aug 6, 2024 •

edited by unfurl-links bot

Loading

JaroslavTulach left a comment

Akirathan commented Aug 7, 2024

WithWarnings uses EnsoHashMap to speed things up #10555

WithWarnings uses EnsoHashMap to speed things up #10555

Conversation

Akirathan commented Jul 15, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

JaroslavTulach left a comment

Choose a reason for hiding this comment

JaroslavTulach Jul 18, 2024

Choose a reason for hiding this comment

Akirathan Aug 5, 2024

Choose a reason for hiding this comment

JaroslavTulach Aug 6, 2024

Choose a reason for hiding this comment

Akirathan Aug 6, 2024

Choose a reason for hiding this comment

Akirathan Aug 6, 2024

Choose a reason for hiding this comment

GregoryTravis Aug 6, 2024

Choose a reason for hiding this comment

Akirathan Aug 6, 2024

Choose a reason for hiding this comment

Akirathan commented Aug 6, 2024 • edited by unfurl-links bot Loading

JaroslavTulach left a comment

Choose a reason for hiding this comment

Akirathan commented Aug 7, 2024

Akirathan commented Jul 15, 2024 •

edited

Loading

Akirathan commented Aug 6, 2024 •

edited by unfurl-links bot

Loading