Compress SourceMapWriter.Fragment data #4965

gzm0 · 2024-03-17T16:38:25Z

We use a similar compression strategy than source maps themselves to reduce the in-memory footprint.

After linking the test suite, residual memory usage is as follows:

what	main [MB]	PR [MB]
sbt overall heap	842	772
backend retained	147	77
frontend retained	215	215

gzm0 · 2024-03-17T16:39:11Z

Draft because:

Needs to wait for 1.16.0 release.
Needs performance benchmarking (touches performance sensitive code).
Needs to produce correct source maps :P

sjrd · 2024-03-17T21:54:53Z

Those numbers look awesome!

Needs to produce correct source maps :P

🤣

gzm0 · 2024-03-18T07:18:01Z

Needs to produce correct source maps :P

It was just an inverted condition. In fact, it produced no source maps :P

gzm0 · 2024-03-23T15:50:04Z

Seems like this is also faster :) (attn: cropped plot, 4 outliers dropped).

EDIT: These are incremental timings (I have not measured batch):

logger-timings.csv

gzm0 · 2024-03-23T15:51:55Z

linker/shared/src/main/scala/org/scalajs/linker/backend/BasicLinkerBackend.scala

@@ -41,9 +41,11 @@ final class BasicLinkerBackend(config: LinkerBackendImpl.Config)
  private[this] var totalModules = 0
  private[this] val rewrittenModules = new AtomicInteger(0)

+  private[this] val fragmentIndex = new SourceMapWriter.Index


Note: We accept leaking old sources / names in incremental runs here (similar to how NameGen leaks as well).

Are we sure that this is only used (for writing) from a single thread at a time? Could you add a comment about that?

Yes, because the Emitter is in fact single threaded. This is probably also something we should fix. I'll make Index thread-safe and see if it affects the benchmarks. If it doesn't, we should probably keep the thread safe version :).

sjrd

The approach definitely looks good overall. I have a few localized comments.

sjrd · 2024-03-25T09:11:24Z

linker/shared/src/main/scala/org/scalajs/linker/backend/javascript/SourceMapWriter.scala

-      if (pendingColumnInGenerated >= 0)
-        doWriteSegment(pendingColumnInGenerated, pendingPos, pendingName)
+      if (pendingColumnInGenerated >= 0) {
+        // Allocate a name string *before* we write a fragment so we can cache it.


I don't understand what this comments is trying to say.

Obsolete with the revert. The idea was that we convert OriginalName to String before we create a fragment so that when we write a fragment, we already have a String, so we're faster.

sjrd · 2024-03-25T09:20:55Z

linker/shared/src/main/scala/org/scalajs/linker/backend/javascript/SourceMapWriter.scala

@@ -39,18 +40,18 @@ object SourceMapWriter {
  private final class NodePosStack {
    private var topIndex: Int = -1
    private var posStack: Array[Position] = new Array(128)
-    private var nameStack: Array[String] = new Array(128)
+    private var nameStack: Array[OriginalName] = new Array(128)


This is potentially problematic from a performance point of view. An array of some AnyVal class is an array of its boxed instances. So at the bytecode level we need to allocate instances of OriginalName to put them in this array.

The benchmarks suggest that this is not such a big deal in this case, or that it's well compensated by the other improvements. If it is the latter case, there is maybe more to gain by avoiding this.

Ah, ah. I did not realize this. I've reverted the first commit that turned this into an original name.

It probably does not show up in the performance profile because this happens pre-cache (almost exclusively). So in an incremental run, we only hit this code path for very little code.

sjrd · 2024-03-25T09:49:39Z

linker/shared/src/main/scala/org/scalajs/linker/backend/javascript/SourceMapWriter.scala

    // Source index field
    if (source eq lastSource) { // highly likely
      buffer(offset) = 'A' // 0 in Base64VLQ
      offset += 1
    } else {
-      val sourceIndex = sourceToIndex(source)
+      val sourceIndex = outIndex.sourceToIndex(source)


There is probably a huge performance win to get here, if we reuse the integer index from the global Index as the key for outIndex. Basically making outIndex a pair of Int -> Int maps rather than of String -> Int maps. And because the keys of such maps would be indices of a reified data structure, we can probably make those Int -> Int maps raw Array[Int]s. IIRC, these map lookups were sill significant in the profiles after all the optimizations, so getting rid of them could have a large impact.

This does not have to be in this PR, though, of course.

sjrd · 2024-03-25T09:51:19Z

linker/shared/src/main/scala/org/scalajs/linker/backend/BasicLinkerBackend.scala

@@ -41,9 +41,11 @@ final class BasicLinkerBackend(config: LinkerBackendImpl.Config)
  private[this] var totalModules = 0
  private[this] val rewrittenModules = new AtomicInteger(0)

+  private[this] val fragmentIndex = new SourceMapWriter.Index


Are we sure that this is only used (for writing) from a single thread at a time? Could you add a comment about that?

linker/shared/src/main/scala/org/scalajs/linker/backend/javascript/SourceMapWriter.scala

sjrd · 2024-03-25T09:54:19Z

linker/shared/src/main/scala/org/scalajs/linker/backend/javascript/SourceMapWriter.scala

+      var nameIndex: Int = 0
+
+      while (buf.hasRemaining()) {
+        (buf.get(): @unchecked) match {


@unchecked is not necessary for Int matches, is it?

@switch would be relevant, though.

Indeed. Fixed.

sjrd · 2024-03-25T12:17:36Z

linker/shared/src/main/scala/org/scalajs/linker/backend/javascript/SourceMapWriter.scala

+        if ((value & ~127) != 0)
+          buffer(offset) = ((value & 127) | 128).toByte
+        else
+          buffer(offset) = (value & 127).toByte


Consider using hexadecimal notation when we use numbers for their bit patterns:

Suggested change

if ((value & ~127) != 0)

buffer(offset) = ((value & 127) | 128).toByte

else

buffer(offset) = (value & 127).toByte

if ((value & ~0x7f) != 0)

buffer(offset) = ((value & 0x7f) | 0x80).toByte

else

buffer(offset) = (value & 0x7f).toByte

sjrd · 2024-03-25T12:32:12Z

linker/shared/src/main/scala/org/scalajs/linker/backend/javascript/SourceMapWriter.scala

+      value >>>= 1
+
+      if (!neg) value
+      else if (value == 0) Int.MinValue


Given what the numbers we read represent, Int.MinValue is not a realistic input for writeRawVLQ. Consider getting rid of this case to avoid one branch. (It might even remove two branches at the assembly level since it will basically become a SELECT.)

We use a similar compression strategy than source maps themselves to reduce the in-memory footprint. After linking the test suite, residual memory usage is as follows: | what | main [MB] | PR [MB] | |-------------------|----------:|--------:| | sbt overall heap | 842 | 772 | | backend retained | 147 | 77 | | frontend retained | 215 | 215 |

gzm0 · 2024-03-31T11:57:33Z

Huh, the updated measurements look suspiciously like the baseline. (I'm willing to attribute the spread to the locking introduced). I'll benchmark just the original first commit (changing String to OriginalName), maybe it makes things faster?

logger-timings.csv

gzm0 · 2024-03-31T12:23:23Z

Ok, this is a bit annoying: It seems my benchmarking setup is insufficiently consistent: The spread I observe seems to be caused by the environment.

However, I think for this PR it does not matter: We have sufficient evidence that it does not make matters worse, and the main goal is to reduce memory consumption, which it does.

legend:

pr original PR (old)
main original baseline (old)
pr-v2 this PR
orig-name just the first commit of the original PR
pr-v2-not-thread-safe this PR with reverted thread safety of Index
main-v2 baseline (again)

logger-timings.csv

gzm0 force-pushed the compress-sm-frags branch from 8d3793e to ad8e4ff Compare March 17, 2024 18:39

gzm0 force-pushed the compress-sm-frags branch from ad8e4ff to dff36b7 Compare March 23, 2024 15:03

gzm0 commented Mar 23, 2024

View reviewed changes

gzm0 marked this pull request as ready for review March 23, 2024 15:52

gzm0 requested a review from sjrd March 23, 2024 15:52

This was referenced Mar 23, 2024

Investigate linker memory consumption #4906

Open

Cleanups to Emitter post transforms #4970

Merged

sjrd requested changes Mar 25, 2024

View reviewed changes

gzm0 force-pushed the compress-sm-frags branch from dff36b7 to 05f39f5 Compare March 31, 2024 11:44

gzm0 requested a review from sjrd March 31, 2024 12:23

sjrd approved these changes Mar 31, 2024

View reviewed changes

sjrd merged commit 3dfcb9a into scala-js:main Mar 31, 2024
3 checks passed

gzm0 deleted the compress-sm-frags branch March 31, 2024 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compress SourceMapWriter.Fragment data #4965

Compress SourceMapWriter.Fragment data #4965

gzm0 commented Mar 17, 2024

gzm0 commented Mar 17, 2024 •

edited

sjrd commented Mar 17, 2024

gzm0 commented Mar 18, 2024

gzm0 commented Mar 23, 2024 •

edited

gzm0 Mar 23, 2024

sjrd Mar 25, 2024

gzm0 Mar 31, 2024

sjrd left a comment

sjrd Mar 25, 2024

gzm0 Mar 31, 2024

sjrd Mar 25, 2024

gzm0 Mar 31, 2024

sjrd Mar 25, 2024

sjrd Mar 25, 2024

sjrd Mar 25, 2024

gzm0 Mar 31, 2024

sjrd Mar 25, 2024

sjrd Mar 25, 2024

gzm0 commented Mar 31, 2024

gzm0 commented Mar 31, 2024

Compress SourceMapWriter.Fragment data #4965

Compress SourceMapWriter.Fragment data #4965

Conversation

gzm0 commented Mar 17, 2024

gzm0 commented Mar 17, 2024 • edited

sjrd commented Mar 17, 2024

gzm0 commented Mar 18, 2024

gzm0 commented Mar 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjrd 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzm0 commented Mar 31, 2024

gzm0 commented Mar 31, 2024

gzm0 commented Mar 17, 2024 •

edited

gzm0 commented Mar 23, 2024 •

edited