Fuse emitting and printing of trees in the backend

[gzm0]

This allows us to use the Emitter's powerful caching mechanism to
directly cache printed trees (as byte buffers) and not cache
JavaScript trees anymore at all.

This reduces in-between run memory usage on the test suite from
1.13 GB (not GiB) to 1.01 GB on my machine (roughly 10%).

Runtime performance (both batch and incremental) is unaffected.

It is worth pointing out, that in order to avoid duplicate caching, we do not cache full class trees anymore.

[gzm0]

Performance measurements

Sorry for the swapped colors between inc and batch.

Incremental (including warmup run)

Full

Cropped (same data)

Batch

Supplemental info

Script to calculate "Full Backend" timing.

library(readr)
library(ggplot2)
library(dplyr)

d_full <- read_csv("logger-timings-batch.csv", col_names = c("variant", "op", "t_ns"), col_types = "ffn")

d_backend <- bind_cols(
  d_full %>% filter(op == "Emitter") %>% select(variant, t_ns),
  d_full %>% filter(op == "BasicBackend: Write result") %>% select(t_ns) %>% rename(t_ns2 = t_ns),
) %>% mutate(op = "Full Backend", t_ns = t_ns + t_ns2, t_ns2 = NULL)

d <- bind_rows(
  d_full %>% filter(grepl('Emitter|BasicBackend', op)),
  d_backend,
)

ggplot(d, aes(x = op, color = variant, y = t_ns)) + geom_boxplot()

logger-timings-batch.csv
logger-timings-inc.csv

[gzm0]

I've had a look at this with YourKit.

Findings:

What	Size [MB]
main: JS trees	232
main: printed trees	120
main: total	352
pr: printed trees	237
absolute savings pr	115
wasted nested trees pr	117

Thoughts:

• The savings are consistent with the overall heap measurements (equal up to rounding error).
• It seems we're wasting a lot of space for nested trees on a codebase that is low on nested invocations (710 cases out of 6538). This is a major concern since JS class heavier codebases will suffer from this more.

[gzm0]

I don't think I'm comfortable proceeding with this given the potential negative impacts depending on the codebase.

Options I see:

• Abandon this idea.
• Stop caching the entire class but only it's parts. @sjrd do you think this might be a viable approach?

[gzm0]

FWIW: I'm going to try and benchmark the second option :)

[sjrd]

• Stop caching the entire class but only it's parts. @sjrd do you think this might be a viable approach?

That is probably viable, yes.

[gzm0]

Numbers look comparable (incremental):

logger-timings.csv

[gzm0]

I have made some progress here.

@sjrd there are two points I'd like your input on:

• Getting proper indentation indeed is not super tricky, what I do not know is how to test it: So far, tree printing was just unit tested (which is/was fine). However, at this point, there are a lot of non-local decisions that go into indenting the blocks that are cached, so it doesn't seem feasible to unit test this :-/ @sjrd any ideas?
• In order to properly deal with semicolons in the printer, I needed to move semicolon printing into the statement (rather than the block context). As a result, we also print a semicolon after the last statement in a block. This leads to quite a bit of (uncompressed) fastlink size increase. Is this acceptable? (relevant commit: 567cb00)

TODOs for me:

• Clean up the last 4 commits.
• Benchmark (both memory and speed)
• Diff output of linked test suite to check it's unchanged.

[gzm0]

Updated benchmarking results (incremental only):

Cropped:

logger-timings.csv

[gzm0]

Note to self: The adjustments to the JS printer are incorrect: There are semicolons after flow-control statements (e.g. if-else chains).

[gzm0]

@sjrd, this and the dependencies (#4920, #4921, #4922) are ready for review.

I suggest you review 22db2f6 first, to get an idea of what the overall thing is doing.

[gzm0]

The CI failure reveals a problem with the Transformed tree: We cannot implement show anymore for a tree (notably not for trees that contain non-PrintedTree chunks).

[gzm0]

Fixing the show (and running scripted locally) reveals another problem: We cannot cache GCC trees that easily.

It seems they keep a reference to their parents (and siblings). So caching them potentially leaks big amounts of memory, but also doesn't actually let us re-use the nodes.

Maybe the best strategy for now is to not cache AST transforms for GCC for now?

[sjrd]

Maybe the best strategy for now is to not cache AST transforms for GCC for now?

That's probably fine. When we run GCC we're not incremental anyway because it isn't. One level of transformation is not going to change much, especially if we have to recreate new Nodes anyway.

[gzm0]

I have added a commit (to be squashed) to not cache GCC trees. TODO: Write a unit test for the GCC backend to link twice consecutively (we should not only have caught this with a scripted test).

[gzm0]

PR for test is here: #4924.

[sjrd]

This seems weird. Now, both sides of the if call compute, and they do not use its result other than to return it. Shouldn't we return only the Boolean then, and let the caller do the compute themselves? That would avoid the tuple and more importantly the extractChangedAndWithGlobals.

[gzm0]

Hum, yes. You have discovered what I have attempted to brush under the rug 😅 If I extract compute after calling trackChanged, org.scalajs.linker.BasicLinkerBackendTest.noInvalidatedModuleInSecondRun fails.

I'm still at the stage of debugging where I think "this cannot happen". So all-in-all, I guess this clearly warrants further investigation :P

[gzm0]

I found the issue: The alternative code was using a short-circuiting or assignment (||=) so the RHS was not always executed. Putting it into a val first fixed the problem. (updated).

[sjrd]

Cool. Looks good! Thanks!