Lazy Diagnostic storage allocation #10852

4e6 · 2024-08-20T16:15:03Z

Pull Request Description

close #10727

Changelog:

add: implement lazy allocation of DiagnosticStorage objects
update: compiler phases to make sure they don't allocate redundant DiagnosticStorage objects
update: cleanup DiagnosticStorage class

Important Notes

As a result, all 10MB of redundant DiagnosticStorage allocations are gone

Before

After

Checklist

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

The documentation has been updated, if necessary.
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.

JaroslavTulach

I am not really happy to see four diagnostics related methods in IR. I'd like to hear some justification before I even try to look at the massive changes around. Can you somehow update Javadoc of the diagnostics related methods in IR to describe why there have to be four and what's different among each other?

JaroslavTulach · 2024-08-23T07:02:31Z

engine/runtime-parser/src/main/java/org/enso/compiler/core/IR.java

+   *
+   * @return the diagnostic storage of this node
+   */
+  DiagnosticStorage getDiagnostics();


There already is diagnostics(). Why do we need another getDiagnostics()?

One can return null. I updated the docs

JaroslavTulach · 2024-08-23T07:03:01Z

engine/runtime-parser/src/main/java/org/enso/compiler/core/IR.java

+   *
+   * @return the copy of diagnostic storage
+   */
+  DiagnosticStorage diagnosticsCopy();


Why don't we just call diagnostics().copy()?

I think this makes more sense if you put it into the context of LazyDiagnosticStorage, which deal with the possibility of null.
Although I would probably already provide a default implementation here:

DiagnosticStorage diagnosticCopy() { if (diagnostics == null) null else diagnostics.copy(); }

Also I would maybe rename it to diagnosticsLazyCopy

JaroslavTulach · 2024-08-23T07:03:22Z

engine/runtime-parser/src/main/java/org/enso/compiler/core/IR.java

+   *
+   * @return the list of diagnostics
+   */
+  List<Diagnostic> diagnosticsList();


Why don't we call diagnostics().toList()?

JaroslavTulach · 2024-08-23T07:14:29Z

engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/LazyDiagnosticStorage.scala

+    _diagnostics = diagnostics
+  }
+
+  override def diagnostics: DiagnosticStorage = {


I am not really sure what _diagnostics: DiagnosticStorage = _, but if I parse the ideas in this file, I would really prefer to get only:

interface IR { DiagnosticsStorage diagnostics(boolean canReturnNull); }

the implementation in LazyDiagnosticsStorage would be:

trait LazyDiagnosticStorage { self: IR => private[this] var d: DiagnosticsStorage = null; override synchronized def diagnostics(boolean canReturnNull) = { if (!canReturnNull && d == null) { d = DiagnosticsStorage(); } d } }

That's all we need to defer allocation of DiagnosticsStorage until needed, in my opinion. The boilerplate of diagnosticsList, getDiagnostics, diagnosticsCopy can be moved elsewhere or preferably disappear.

_diagnostics: DiagnosticStorage = _

represents the actual allocated DiagnosticStorage and _ means null in this context. You call it d he calls it _diagnostics which is more in line with Scala's way of defining custom setters/getters. I think this is fine.
Also you would have to canReturnNull all over the place and I don't think this is any better.

With this approach

ir.diagnostics(canReturnNull = true).copy()

can throw NPE, that's why there is a to DRY the null checking logic

ir.diagnosticsCopy()

I think I can remove the extra methods from the IR interface and make them more implementation-specific.

With this approach

ir.diagnostics(canReturnNull = true).copy()

can throw NPE, that's why there is a to DRY the null checking logic

In order to be DRY for internal stuff you duplicate a single publicly visible method four times! That's not DRY! To be DRY you'd write a helper method somewhere (hidden enough from regular users of the IR) and used it in all the internal implementations to avoid NPE:

public static DiagnosticsStorage copyOrNull(DiagnosticsStorage sOrNull) { return sOrNull == null ? null : sOrNull.copy(); }

and then called it as DiagnosticsStorage.copyOrNull(ir.diagnostics(true)).

You call it d he calls it

I don't care about any of the proposed names. I just believe four methods related to diagnostics in IR is 75% too much. I believe single DiagnosticsStorage diagnotics(boolean canReturnNull) would be enough to satisfy all use-cases and keep conceptual surface (some may call it DRY) smaller.

Akirathan · 2024-08-23T08:37:57Z

...-integration-tests/src/test/scala/org/enso/compiler/test/core/ir/DiagnosticStorageTest.scala

@@ -42,115 +41,5 @@ class DiagnosticStorageTest extends CompilerTest {
      diagnostics.toList should contain(mkDiagnostic("b"))
      diagnostics.toList should contain(mkDiagnostic("c"))
    }
-
-    "mapping across the diagnostics to produce a new sequence" in {


What is the reason for removing all these tests?

They were testing the logic that was merely delegating to the underlying list of DiagnosticStorage. This logic is no longer used

Akirathan · 2024-08-23T08:39:19Z

engine/runtime-integration-tests/src/test/scala/org/enso/compiler/test/CompilerTest.scala

@@ -151,23 +151,23 @@ trait CompilerRunner {
      */
    def asMethod: definition.Method = {
      new definition.Method.Explicit(
-        Name.MethodReference(


How is this change related to diagnostic storage?

During the compilation, the Explicit node is always created from the Binding copying the accompanying diagnostic, location, etc. The constructor was updated to DRY it.

hubertp · 2024-08-23T09:03:35Z

engine/runtime-parser/src/main/java/org/enso/compiler/core/IR.java

+   *
+   * @return the copy of diagnostic storage
+   */
+  DiagnosticStorage diagnosticsCopy();


I think this makes more sense if you put it into the context of LazyDiagnosticStorage, which deal with the possibility of null.
Although I would probably already provide a default implementation here:

DiagnosticStorage diagnosticCopy() { if (diagnostics == null) null else diagnostics.copy(); }

Also I would maybe rename it to diagnosticsLazyCopy

hubertp · 2024-08-23T09:06:49Z

engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/LazyDiagnosticStorage.scala

+    _diagnostics = diagnostics
+  }
+
+  override def diagnostics: DiagnosticStorage = {


_diagnostics: DiagnosticStorage = _

represents the actual allocated DiagnosticStorage and _ means null in this context. You call it d he calls it _diagnostics which is more in line with Scala's way of defining custom setters/getters. I think this is fine.
Also you would have to canReturnNull all over the place and I don't think this is any better.

hubertp · 2024-08-23T09:07:07Z

engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/LazyDiagnosticStorage.scala

+    _diagnostics
+  }
+
+  override def diagnosticsList: List[Diagnostic] = {


Oh, now I get it. You essentially have all those methods to deal with the possibility of null and not repeat the logic all over the place.

I believe so. Thus in order to simplify SPI (e.g. implementations re-written by Dmitry in this PR), we have complicated Client API. I see it as a completely opposite approach than the right one - the usage of IR should be simple. The implementation needn't be - API authors have to live thru their pain.

hubertp · 2024-08-23T09:11:32Z

...egration-tests/src/test/scala/org/enso/compiler/test/pass/analyse/DataflowAnalysisTest.scala

@@ -356,31 +356,31 @@ class DataflowAnalysisTest extends CompilerTest {
    val plusSymbol       = mkDynamicDep("+")

    // The Identifiers
-    val methodId           = mkStaticDep(method.getId())


Please leave those with brackets. getId may involve side-effects (as in, actually inferring the new id) and as such should require brackets.

When talking about getId() - there used to be a PR that saved some memory by not allocating UUIDs. Have that PR need for different versions of getId(), @hubertp? If not, then it might be an example to follow when making DiagnosticStorage lazy.

JaroslavTulach

Looks like diagnosticsCopy and diagnosticsList are gone from IR and are only in LazyDiagnosticsStorage - more hidden, but still visible. I believe those methods shouldn't be visible at all. Moreover the initialization of _diagnostics field isn't thread safe. So I don't believe I can approve this change - but I have "requested changes"...

Approval just to _un-"request changes". Dmitry please find approval from someone else, don't count mine as +1.

engine/runtime-parser/src/main/java/org/enso/compiler/core/IR.java

JaroslavTulach · 2024-08-23T14:16:22Z

engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/LazyDiagnosticStorage.scala

+    _diagnostics
+  }
+
+  def diagnosticsList: List[Diagnostic] = {


Make protected

JaroslavTulach · 2024-08-23T14:16:51Z

engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/LazyDiagnosticStorage.scala

+    if (_diagnostics eq null) Nil else _diagnostics.toList
+  }
+
+  def diagnosticsCopy: DiagnosticStorage = {


Can it also be protected?

No, it is used in compiler passes.

JaroslavTulach · 2024-08-23T14:17:45Z

engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/LazyDiagnosticStorage.scala

+
+  private[this] var _diagnostics: DiagnosticStorage = _
+
+  protected def diagnostics_=(diagnostics: DiagnosticStorage): Unit = {


Who's calling setter (this is setter, right)? There was no setter before, so I don't think we need a method for it now.

We need a setter when creating an IR node and want to pass some existing diagnostic storage.

JaroslavTulach · 2024-08-23T14:18:23Z

engine/runtime-parser/src/main/scala/org/enso/compiler/core/ir/LazyDiagnosticStorage.scala

+
+  override def getDiagnostics: DiagnosticStorage = {
+    if (_diagnostics eq null) {
+      _diagnostics = new DiagnosticStorage()


This is not ready for multi threaded access. Are we OK with that?

The entire compilation logic is not thread-safe. Compilation jobs ensure that the process is single-threaded

Why are we then using immutable IR and not modifying IR in place? Ó the wonders of like to be functional programming!

4e6 · 2024-08-23T15:42:25Z

@JaroslavTulach GitHub allows reviewer to dismiss their review

…java Co-authored-by: Jaroslav Tulach <jaroslav.tulach@enso.org>

I don't want to block the performance improvement

4e6 added 3 commits August 19, 2024 21:31

DRAFT: lazy diagnostic storage

f49b55d

DRAFT: lazy copy

2239f0f

fix: disgnostic persistence

0176d6a

4e6 added the CI: No changelog needed Do not require a changelog entry for this PR. label Aug 20, 2024

4e6 self-assigned this Aug 20, 2024

4e6 force-pushed the wip/db/10727-diagnosticstorage-allocated branch from 444312f to 76da204 Compare August 21, 2024 13:57

4e6 added 12 commits August 21, 2024 14:57

misc: IR interface

76da204

misc: format

5e61194

misc: simplify diagnostic storage

b0f50d5

misc: IrToTruffle skip reduntant diagnostics creation

386a25e

fix: gather diagnostics test

17129dd

misc: cleanup

d000fcc

misc: cleanup profiling

02695af

misc: scalafmt

140a5d6

misc: refactor ir interface

6d165f5

Merge branch 'develop' into wip/db/10727-diagnosticstorage-allocated

3777082

fix: after merge

daa6e58

fix: tests

7308bd6

4e6 marked this pull request as ready for review August 22, 2024 10:08

4e6 requested review from JaroslavTulach, hubertp and Akirathan as code owners August 22, 2024 10:08

4e6 added 2 commits August 22, 2024 11:21

misc: unnecessary inheritdoc

ebf06e8

misc: tidy up

bd42af0

JaroslavTulach requested changes Aug 23, 2024

View reviewed changes

JaroslavTulach reviewed Aug 23, 2024

View reviewed changes

Akirathan reviewed Aug 23, 2024

View reviewed changes

hubertp approved these changes Aug 23, 2024

View reviewed changes

hubertp reviewed Aug 23, 2024

View reviewed changes

4e6 added 2 commits August 23, 2024 11:27

update: remove extra diagnostic methods from IR interface

4c09e26

update: revert getId

3f8534c

4e6 requested a review from JaroslavTulach August 23, 2024 10:47

doc: update javadoc

05e01d7

JaroslavTulach previously approved these changes Aug 23, 2024

View reviewed changes

JaroslavTulach self-requested a review August 23, 2024 14:25

4e6 and others added 2 commits August 23, 2024 16:43

Update engine/runtime-parser/src/main/java/org/enso/compiler/core/IR.…

33a4a29

…java Co-authored-by: Jaroslav Tulach <jaroslav.tulach@enso.org>

misc: cleanup

7fa045e

4e6 added the CI: Ready to merge This PR is eligible for automatic merge label Aug 23, 2024

mergify bot merged commit 3bfe963 into develop Aug 23, 2024
42 checks passed

mergify bot deleted the wip/db/10727-diagnosticstorage-allocated branch August 23, 2024 17:48

unfurl-links bot mentioned this pull request Aug 24, 2024

Don't use System.out.print in Standard.Test - use IO! #10028

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy Diagnostic storage allocation #10852

Lazy Diagnostic storage allocation #10852

4e6 commented Aug 20, 2024 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Aug 23, 2024

4e6 Aug 23, 2024

JaroslavTulach Aug 23, 2024

hubertp Aug 23, 2024

JaroslavTulach Aug 23, 2024

JaroslavTulach Aug 23, 2024 •

edited

Loading

hubertp Aug 23, 2024

4e6 Aug 23, 2024

4e6 Aug 23, 2024

JaroslavTulach Aug 23, 2024

Akirathan Aug 23, 2024

4e6 Aug 23, 2024

Akirathan Aug 23, 2024

4e6 Aug 23, 2024

hubertp Aug 23, 2024

hubertp Aug 23, 2024

hubertp Aug 23, 2024

JaroslavTulach Aug 23, 2024 •

edited

Loading

hubertp Aug 23, 2024

JaroslavTulach Aug 23, 2024 •

edited

Loading

JaroslavTulach left a comment

JaroslavTulach Aug 23, 2024

JaroslavTulach Aug 23, 2024

4e6 Aug 23, 2024

JaroslavTulach Aug 23, 2024

4e6 Aug 23, 2024

JaroslavTulach Aug 23, 2024

4e6 Aug 23, 2024

JaroslavTulach Aug 23, 2024

4e6 commented Aug 23, 2024


		private[this] var _diagnostics: DiagnosticStorage = _

		protected def diagnostics_=(diagnostics: DiagnosticStorage): Unit = {

Lazy Diagnostic storage allocation #10852

Lazy Diagnostic storage allocation #10852

Conversation

4e6 commented Aug 20, 2024 • edited Loading

Pull Request Description

Important Notes

Before

After

Checklist

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

JaroslavTulach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

4e6 commented Aug 23, 2024

4e6 commented Aug 20, 2024 •

edited

Loading

JaroslavTulach Aug 23, 2024 •

edited

Loading

JaroslavTulach Aug 23, 2024 •

edited

Loading

JaroslavTulach Aug 23, 2024 •

edited

Loading