Simplify GROWABLE_HEAP transform

[RReverser]

Similar to #24291, but for the transform handling -pthread + -sALLOW_MEMORY_GROWTH.

In the first commit I added a codesize variation to commit current sizes, and in the second you can see that this transform actually saves the total size in addition to making acorn-optimizer simpler.

[sbc100]

Nice!

[RReverser]

Converted to draft because I thought of some cases where this change can be problematic without further work, even though those cases are not yet covered by tests.

SAFE_HEAP one should be still good to go.

[RReverser]

Converted to draft because I thought of some cases where this change can be problematic without further work, even though those cases are not yet covered by tests.

Converted to ready for review because turns out it's not a regression, the edge case I was thinking of (memory growth + ASAN/SAFE_HEAP) is already not working as expected and will be addressed separately in #24309.

[RReverser]

I want to do a bit more testing here just in case, so holding off merging for now.

[RReverser]

Yessss. After investigating #24309, I realised another issue with this refactoring, and I knew this should be failing in some cases, but couldn't make it break any tests.

Turns out, it's tricky to trigger that edge case due to pthreads involved and due to Emscripten accessing the heap in a lot of functions (and, thus, updating it even before it's actually needed), but finally managed to craft a test that breaks after this transform.

One of those rare feelings when things work but they really shouldn't 😅 I'll submit a new test either in this PR or separately.

[RReverser]

Ok the tests look pretty different now because majority of Emscripten APIs that are implemented in JS touch HEAP in one way or another, and so even logging via printf/puts or macro like EM_ASM would hide the issue by updating the heap bindings too early.

I had to drop all the way down to EM_JS + atomics, which successfully broke all those tests as expected (showing that memory view is not updated on the first access), and subsequently fixed them by changing transform from GROWABLE_HEAP(HEAPxx) to (GROWABLE_HEAP(), HEAPxx) which ensures that the heap value is always retrieved after updating the views, and not evaluated before that.

@kripken @sbc100 I'm waiting for the CI to see if this uncovered anything else, but otherwise should be ready for another look.

[sbc100]

Ok the tests look pretty different now because majority of Emscripten APIs that are implemented in JS touch HEAP in one way or another, and so even logging via printf/puts or macro like EM_ASM would hide the issue by updating the heap bindings too early.

I had to drop all the way down to EM_JS + atomics, which successfully broke all those tests as expected (showing that memory view is not updated on the first access), and subsequently fixed them by changing transform from GROWABLE_HEAP(HEAPxx) to (GROWABLE_HEAP(), HEAPxx) which ensures that the heap value is always retrieved after updating the views, and not evaluated before that.

I don't understand why you can't use use the return value of GROWABLE_HEAP(HEAPxx).. can you explain a little more? Is the problem that incoming argument to GROWABLE_HEAP would somehow be wrong?

@kripken @sbc100 I'm waiting for the CI to see if this uncovered anything else, but otherwise should be ready for another look.

[RReverser]

I don't understand why you can't use use the return value of GROWABLE_HEAP(HEAPxx).. can you explain a little more? Is the problem that incoming argument to GROWABLE_HEAP would somehow be wrong?

Arguments are evaluated before the function itself, which is what I was trying to demonstrate & catch with the updated tests.

If you do

let gotHeap = GROWABLE_HEAP(HEAP8);

then it's equivalent to

let gotHeap = (function GROWABLE_HEAP(arr) {
  // inside GROWABLE_HEAP:
  HEAP8 = new Uint8Array(...);
  HEAP16 = ...;
  ...;
  return arr;
})(HEAP8);

which in turn is like

let arr = HEAP8;
HEAP8 = new Uint8Array(...);
HEAP16 = ...;
...;
let gotHeap = arr; // the old value of HEAP8, not the one from assignment above

---

Meanwhile, doing

let gotHeap = (GROWABLE_HEAP(), HEAP8);

is like

HEAP8 = new Uint8Array(...);
HEAP16 = ...;
...;
let gotHeap = HEAP8;

which is exactly what we want.

[sbc100]

I see. In that case perhaps we can rename GROWABLE_HEAP() to something more meaningful? "MAYBE_UPDATE_HEAP_VIEWS" (seems a bit lot). CHECK_MEM_GROWTH (better?)?

[RReverser]

In that case perhaps we can rename GROWABLE_HEAP() to something more meaningful? "MAYBE_UPDATE_HEAP_VIEWS" (seems a bit lot). CHECK_MEM_GROWTH (better?)?

Yeah, either is fine by me. I was thinking of maybeUpdateMemoryViews() to be in line with updateMemoryViews() but wasn't sure if worth it.

[RReverser]

In that case perhaps we can rename GROWABLE_HEAP() to something more meaningful? "MAYBE_UPDATE_HEAP_VIEWS" (seems a bit lot). CHECK_MEM_GROWTH (better?)?

Yeah, either is fine by me. I was thinking of maybeUpdateMemoryViews() to be in line with updateMemoryViews() but wasn't sure if worth it.

I'll just do this, especially since this function is no longer exposed as runtime symbol and doesn't need to have the uppercase name.

[sbc100]

Yeah I guess there is no reason to do with all caps here.

[sbc100]

Since this symbol will appear a lot in unminified output is it worth picking a short-ish name? Maybe not?

[RReverser]

Eh, personally I'd prioritise debuggability over code size for unminified JS.

[RReverser]

FWIW I'll probably merge / rename those functions a bit more as part of fixing #24309 anyway.

[RReverser]

CI looks good and I feel more confident in this change now 🤞

[RReverser]

FAIL [0.002s]: test_async_hello_v8 (test_core.instance) looks like a broken test on main branch.

[kripken]

Trying to read the changes since my last review, I have a problem that I've seen in @sbc100 's PRs as well with force-pushes... how can I see only the last changes? The last "compare" button shows what appears to be a merge commit from main, and nothing else, and otherwise I see 13 force-pushed commits but I can't tell which are different from before and which (if any) are new compared to before.

In general I think using normal merge commits, not force-pushes, avoids such problems. Is there another way to read the diff here?

[sbc100]

Trying to read the changes since my last review, I have a problem that I've seen in @sbc100 's PRs as well with force-pushes... how can I see only the last changes? The last "compare" button shows what appears to be a merge commit from main, and nothing else, and otherwise I see 13 force-pushed commits but I can't tell which are different from before and which (if any) are new compared to before.

In general I think using normal merge commits, not force-pushes, avoids such problems. Is there another way to read the diff here?

It looks like he did squash the commits so if you want to do an incremental review you could just look at the later commits.

Obviously its not ideal because (a) you have to trust the earlier commits are not updated and (b) you need to keep track or remember which commit you last reviewed. But as somehow a very much a rebase workflow person, I'm hesitant to force folks into a merge workflow.

[sbc100]

Personally I always like to review the whole change each time, just to remind myself what its doing as a whole.

[RReverser]

Hm I don't think I did a squash, just the "update branch with rebase" via Github. Those are annoying too on Github, but the actual commits should be still the same as before.

@kripken this last commit is the only change since last review: 54031be

[kripken]

Ok, thanks, last commit looks good.