Store whole IR.Module in .bindings cache #8924

JaroslavTulach · 2024-02-01T12:38:03Z

Pull Request Description

Fixes #8639 by storing whole IR.Module inside of library .bindings cache and using them instead of loading caches from individual .ir files. The --compile command is modified to not produce .ir caches - they are already contained in .bindings files.

Checklist

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

All code follows the
Scala,
Java,
style guides.
All code has been tested:
- All tests continue to pass
- Benchmarks are good as they show 3,5% speed up. One more benchmarks run.
- Use mmapped buffers only for "large files" like .bindings from Table or Base - restricted in c061494

JaroslavTulach · 2024-02-01T12:45:30Z

The .bindings files get larger, obviously. The original sizes were:

enso$ find built-distribution/ -type f | grep bindings$ | xargs wc -c
  30416 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Visualization/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Visualization.bindings
   1341 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Google_Api/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Google_Api.bindings
  52768 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/AWS/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/AWS.bindings
   9655 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Searcher/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Searcher.bindings
   4310 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Geo/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Geo.bindings
  19415 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Image/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Image.bindings
   8678 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Examples/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Examples.bindings
  29898 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Test_New/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Test_New.bindings
 926343 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Base/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Base.bindings
 449874 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Database/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Database.bindings
 377276 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Table/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Table.bindings
  23754 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Test/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Test.bindings
1933728 total

e.g. 2MB, but now the size is 66MB:

enso$ find built-distribution/ -type f | grep bindings$ | xargs wc -c
   37876 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Searcher/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Searcher.bindings
   94060 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Geo/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Geo.bindings
  784829 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Image/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Image.bindings
  175137 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Examples/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Examples.bindings
 2208525 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Test_New/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Test_New.bindings
27094576 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Base/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Base.bindings
16163637 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Database/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Database.bindings
17308777 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Table/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Table.bindings
 2694845 built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Test/0.0.0-dev/.enso/cache/bindings/0.0.0-dev/Standard/Test.bindings
66562262 total

…Manager

JaroslavTulach · 2024-02-03T06:49:43Z

bb1272d shall fix #8698 as verified by another benchmark run:

Down by 3,5% to 2600ms in empty startup (without using any Standard libraries). The startup with Standard libraries takes 1s longer - e.g. 1s is the cost of processing those libraries.

JaroslavTulach · 2024-02-03T06:57:56Z

engine/runtime/src/main/scala/org/enso/interpreter/runtime/SerializationManager.scala

-    collection.concurrent.TrieMap[QualifiedName, Future[Boolean]]()
-
-  /** The thread pool that handles serialization. */
-  private val pool: ThreadPoolExecutor = new ThreadPoolExecutor(


This PR moves all the threading mangling into SerializationPool to separate the work with threads from the remaining logic inside of SerializationManager.

JaroslavTulach · 2024-02-05T06:03:43Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/SerializationPool.java

+import java.util.logging.Level;
+import org.enso.pkg.QualifiedName;
+
+final class SerializationPool {


SerializationPool contains all the threading logic of original SerializationManager. It replaces Thread.sleep with Object.wait and it trades the parallel executor for a newSingleThreadExecutor. Otherwise the design with isSerializing and isWaitingForSerialization remains unchanged.

JaroslavTulach · 2024-02-05T06:18:17Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/ThreadExecutors.java

+      shutdownPools();
+    }
+    synchronized (threads) {
+      for (var t : threads.keySet()) {


Looks like this fixes the problem where context.close() complains about dangling threads. Probably shutdownPool() stops the pools, but the threads are left to finish later. Sometimes the context.close() is called faster and some of the threads are not yet finished.

Originally with Thread.sleep(100) this situation wasn't happening (too frequently), but now with wait/notify it got way more common. This t.join() seems to fix that problematic situation altogether.

engine/runtime/src/main/scala/org/enso/interpreter/runtime/SerializationManager.scala

JaroslavTulach · 2024-02-05T06:25:46Z

engine/runtime/src/test/java/org/enso/interpreter/caches/ModuleCacheTest.java


    var meta = new ModuleCache.Metadata("hash", "code", CompilationStage.AFTER_CODEGEN.toString());
-    var cachedIr = module.getCache().deserialize(ensoCtx, arr, meta, null);
+    var cachedIr = mc.deserialize(ensoCtx, ByteBuffer.wrap(arr), meta, null);


Deserialization now works with ByteBuffer, not byte[] to allow mmapped files.

JaroslavTulach · 2024-02-05T06:28:26Z

lib/java/persistance/src/main/java/org/enso/persist/Persistance.java

+   * @return the read object
+   * @throws java.io.IOException when an I/O problem happens
+   */
+  public static <T> Reference<T> read(ByteBuffer buf, Function<Object, Object> readResolve)


New method to allow us to pass in mmapped buffer. The internals were already prepared for working with ByteBuffer.

JaroslavTulach · 2024-02-05T06:31:20Z

engine/runtime/src/main/java/org/enso/interpreter/caches/ModuleCache.java

-public final class ModuleCache extends Cache<ModuleCache.CachedModule, ModuleCache.Metadata> {
-
+public final class ModuleCache
+    implements Cache.Spi<ModuleCache.CachedModule, ModuleCache.Metadata> {


Cache is not final class. All the cache implementations now implement the Spi interface and create instance of Cache with its create method.

JaroslavTulach · 2024-02-05T06:31:56Z

engine/runtime/src/main/java/org/enso/interpreter/caches/CacheUtils.java

+
+import com.oracle.truffle.api.TruffleFile;
+
+final class CacheUtils {


Methods of Cache that are more helper utilities.

JaroslavTulach · 2024-02-05T10:15:03Z

I believe the code is ready to be reviewed. There seems to be some suspicious StackOverflowError in the CI logs/runs. Not sure how frequent/dangerous it can be. I haven't seen the StackOverflowError locally yet.

engine/runtime-compiler/src/main/scala/org/enso/compiler/Compiler.scala

engine/runtime/src/main/java/org/enso/interpreter/caches/Cache.java

engine/runtime/src/main/java/org/enso/interpreter/runtime/SerializationPool.java

hubertp · 2024-02-05T22:01:13Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/SerializationPool.java

+        // Bound the waiting loop
+        int maxCount = 60;
+        int counter = 0;
+        while (this.hasJobsRemaining() && counter < maxCount) {


log when max count was reached?

This is relict of the original code. I kept it, but I have no idea what it was trying to achieve. In my opinion one shall either:

abort the serialization - in such case there is executor.shutdownNow() method

wait for all serializations to finish - in such case there is executor.shutdown()

One calls either first or other method. I have no idea why the shutdown sequence is so complicated.

engine/runtime/src/main/java/org/enso/interpreter/runtime/SerializationPool.java

hubertp · 2024-02-05T22:10:36Z

engine/runtime/src/main/java/org/enso/interpreter/runtime/TruffleCompilerContext.java

+      pool.abort(toQualifiedName(libraryName));
+      return scala.Option.empty();
+    } else {
+      pool.waitWhileSerializing(toQualifiedName(libraryName));


is this safe? nothing stops context from serializing the library in between waiting and loading the cache? Not that it is likely to happen, but stil.

It is as "safe" as the original implementation - I haven't changed the logic. True, the original code was probably not really multithreading ready. The fact that it was using Thread.sleep for coordination of threads speaks for itself.

I haven't attempted to make the code bulletproof. That would require bigger rewrite. Btw. can the code even be really safe when multiple processes access the caches at the same time? That might require some co-ordination with filesystem. What goals do we have for the caching infrastructure?

My current implementation implies:

caches are written down in a background executor thread

reading the caches happens in a caller thread and waits until the background thread finishes

If we want more, we should probably sit down and collect other requirements/constraints. Then rewrite the implementation.

It would be great if the coordination of filesystem access to caches was encapsulated in a single class like SerializationPool. To replace this waitWhileSerializing method we would need something like:

public <T> T readCaches(QualifiedName name, Callbable<T> action) {...}

that would prevent serialization of name key while executing the action. I can try to redesign that.

engine/runtime/src/test/java/org/enso/compiler/SerializationManagerTest.java

Co-authored-by: Hubert Plociniczak <[email protected]>

…nged its return type

JaroslavTulach · 2024-02-06T06:33:25Z

There seems to be some suspicious StackOverflowError in the CI logs/runs...

The way to reproduce the exception is to sbt buildEngineDistribution and then:

enso$ ./built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/bin/enso --ir-caches --no-compile-dependencies --no-global-cache --compile ./built-distribution/enso-engine-0.0.0-dev-linux-amd64/enso-0.0.0-dev/lib/Standard/Google_Api/0.0.0-dev/
[ERROR] [2024-02-06T06:43:12+01:00] [org.enso.runner.Main] Unexpected internal error
org.graalvm.polyglot.PolyglotException: Resource exhausted: Stack overflow
~/NetBeansProjects/enso/enso$

Fixed by 947824e - when deserialization yields IOException, then ensureParsed, but without using caches.

…n ensureParsed again

… .bindings files.

JaroslavTulach · 2024-02-07T04:49:51Z

There is an error during serialization of suggestions however the exception seems to be swallowed in spite of being logged via TruffleLogger (ccing @hubertp). Trying to reveal the stacktrace with old plain good printStackTrace: 76f47a7

Update: With the help of printStackTrace (sic!) the exception got printed. Let's see if 11776a6 fixes the problem.

Akirathan

The changes seem generally good and understandable. I have just left very minor nitpicks.

engine/runtime/src/main/java/org/enso/interpreter/runtime/SerializationPool.java

JaroslavTulach · 2024-02-08T03:56:25Z

engine/runtime-compiler/src/main/scala/org/enso/compiler/phase/ImportResolver.scala

@@ -179,7 +179,7 @@ class ImportResolver(compiler: Compiler) {
            u.compilationStage(CompilationStage.INITIAL)
          }
        )
-        compiler.ensureParsed(mod)
+        compiler.ensureParsed(mod, false)


Don't use caches to avoid this NPE.

JaroslavTulach · 2024-02-08T03:58:57Z

Benchmarks continue to look fine:

There continues to be 100ms speedup.

JaroslavTulach · 2024-02-12T08:46:22Z

First CI failures are starting to appear after integration of this change: https://github.com/enso-org/enso/actions/runs/7869109006/job/21467537020?pr=9024#step:8:974

.FileSystemException: built-distribution\enso-engine-2024.1.1-dev-windows-amd64\enso-2024.1.1-dev\lib\Standard\Base\2024.1.1-dev.enso\cache\bindings\2024.1.1-dev\Standard\Base.bindings: The process cannot access the file because it is being used by another process

I wish I knew what is the other process?

GitHub
Don't print stacktraces of IOException · enso-org/enso@f516695
Hybrid visual and textual functional programming. Contribute to enso-org/enso development by creating an account on GitHub.

Store whole IR.Module in .bindings cache

6803429

JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Feb 1, 2024

JaroslavTulach self-assigned this Feb 1, 2024

JaroslavTulach requested review from 4e6, hubertp and Akirathan as code owners February 1, 2024 12:38

JaroslavTulach marked this pull request as draft February 1, 2024 12:38

JaroslavTulach added 5 commits February 1, 2024 14:08

Shield against NPE

20416bf

Nicer formatting

3331219

Using ByteBuffer instead of byte[] in caches

8dfcac9

Let Persistace.read accept ByteBuffer

05858a6

Reading the cache in as MappedByteBuffer

7be0865

enso-bot bot mentioned this pull request Feb 2, 2024

Use single cache file per library #8639

Closed

JaroslavTulach added 4 commits February 2, 2024 08:09

Clarity: Splitting Cache API and SPI into separate classes

ba07db8

Single throws ClassNotFoundException is enough

6a10a20

Replace writable Cache fields with methods in Spi

b7fd968

Cache is a final class

e570e37

JaroslavTulach force-pushed the wip/jtulach/CachePerLibrary_8639 branch from 3f41ecd to e570e37 Compare February 2, 2024 10:22

JaroslavTulach added 5 commits February 2, 2024 11:31

No need for special Impl inner classes

667260c

No need for Cache.Metadata interface

22ac510

Eliminate one need for SerializationManager.apply

c40bc2f

Less calls to serializationManager.deserialize

09abdc0

Introducing SerializationPool to deal with threading of Serialization…

300972d

…Manager

JaroslavTulach linked an issue Feb 3, 2024 that may be closed by this pull request

Avoid sleep(100) when shutting down (and measuring startup) #8698

Closed

JaroslavTulach added 2 commits February 3, 2024 07:22

Let SerializationPool use QualifiedName as key

0f53896

Removing Thread.sleep from Serialization manager code

bb1272d

JaroslavTulach marked this pull request as ready for review February 3, 2024 06:49

JaroslavTulach commented Feb 3, 2024

View reviewed changes

JaroslavTulach added CI: Clean build required CI runners will be cleaned before and after this PR is built. and removed CI: Keep up to date Automatically update this PR to the latest develop. labels Feb 5, 2024

JaroslavTulach commented Feb 5, 2024

View reviewed changes

engine/runtime/src/main/scala/org/enso/interpreter/runtime/SerializationManager.scala Show resolved Hide resolved

JaroslavTulach commented Feb 5, 2024

View reviewed changes

hubertp reviewed Feb 5, 2024

View reviewed changes

JaroslavTulach and others added 2 commits February 6, 2024 05:29

Use underscore

1577744

Co-authored-by: Hubert Plociniczak <[email protected]>

There is no library name anymore as deserializeSuggestions method cha…

c729c74

…nged its return type

enso-bot bot mentioned this pull request Feb 6, 2024

Language Server breaks on page refresh. #8689

Closed

When recovering from deserialization IOException, don't useCaches whe…

947824e

…n ensureParsed again

hubertp approved these changes Feb 6, 2024

View reviewed changes

JaroslavTulach added 3 commits February 7, 2024 05:30

Only mmap files larget than 3MB - e.g. mostly .bindings files

c061494

Don't generate .ir files when compiling a library. They are stored in…

bbd6d99

… .bindings files.

Make sure the stacktrace isn't lost

76f47a7

Compute suggestions first before (possibly) overriding .bindings file

11776a6

Akirathan approved these changes Feb 7, 2024

View reviewed changes

engine/runtime/src/main/java/org/enso/interpreter/runtime/SerializationPool.java Outdated Show resolved Hide resolved

engine/runtime/src/main/java/org/enso/interpreter/runtime/SerializationPool.java Show resolved Hide resolved

JaroslavTulach added 2 commits February 8, 2024 04:51

Improving javadoc & co.

5a4bfa9

Avoid using caches when reparsing

a2b2945

JaroslavTulach commented Feb 8, 2024

View reviewed changes

JaroslavTulach merged commit 9a91b7b into develop Feb 9, 2024
28 checks passed

JaroslavTulach deleted the wip/jtulach/CachePerLibrary_8639 branch February 9, 2024 03:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store whole IR.Module in .bindings cache #8924

Store whole IR.Module in .bindings cache #8924

JaroslavTulach commented Feb 1, 2024 •

edited

Loading

JaroslavTulach commented Feb 1, 2024

JaroslavTulach commented Feb 3, 2024 •

edited

Loading

JaroslavTulach Feb 3, 2024

JaroslavTulach Feb 5, 2024

JaroslavTulach Feb 5, 2024 •

edited

Loading

JaroslavTulach Feb 5, 2024

JaroslavTulach Feb 5, 2024 •

edited

Loading

JaroslavTulach Feb 5, 2024

JaroslavTulach Feb 5, 2024

JaroslavTulach commented Feb 5, 2024

hubertp Feb 5, 2024

JaroslavTulach Feb 6, 2024 •

edited

Loading

hubertp Feb 5, 2024

JaroslavTulach Feb 6, 2024

JaroslavTulach Feb 6, 2024

JaroslavTulach commented Feb 6, 2024 •

edited

Loading

JaroslavTulach commented Feb 7, 2024 •

edited

Loading

Akirathan left a comment

JaroslavTulach Feb 8, 2024

JaroslavTulach commented Feb 8, 2024

JaroslavTulach commented Feb 12, 2024 •

edited by unfurl-links bot

Loading


		import com.oracle.truffle.api.TruffleFile;

		final class CacheUtils {

Store whole IR.Module in .bindings cache #8924

Store whole IR.Module in .bindings cache #8924

Conversation

JaroslavTulach commented Feb 1, 2024 • edited Loading

Pull Request Description

Checklist

JaroslavTulach commented Feb 1, 2024

JaroslavTulach commented Feb 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach commented Feb 5, 2024

Choose a reason for hiding this comment

JaroslavTulach Feb 6, 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

JaroslavTulach commented Feb 6, 2024 • edited Loading

JaroslavTulach commented Feb 7, 2024 • edited Loading

Akirathan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaroslavTulach commented Feb 8, 2024

JaroslavTulach commented Feb 12, 2024 • edited by unfurl-links bot Loading

JaroslavTulach commented Feb 1, 2024 •

edited

Loading

JaroslavTulach commented Feb 3, 2024 •

edited

Loading

JaroslavTulach Feb 5, 2024 •

edited

Loading

JaroslavTulach Feb 5, 2024 •

edited

Loading

JaroslavTulach Feb 6, 2024 •

edited

Loading

JaroslavTulach commented Feb 6, 2024 •

edited

Loading

JaroslavTulach commented Feb 7, 2024 •

edited

Loading

JaroslavTulach commented Feb 12, 2024 •

edited by unfurl-links bot

Loading