emmalloc: Add an option to not define the standard exports #20487

kripken · 2023-10-18T23:48:22Z

With this PR if emmalloc.c is built with -DEMMALLOC_NO_STD_EXPORTS then we do
not define malloc, free, etc. That means we only provide emmalloc_malloc,
emmalloc_free, etc., the prefixed versions. They can then be used alongside another
malloc impl.

This will be useful in a later PR that adds a two-tiered allocator: a fast multithreaded
one, and underneath it emmalloc, which will function as the "system allocator" for it.
That is, emmalloc will play the role of VirtualAlloc on windows or mmap on POSIX,
a way for the main allocator to get system memory. (We can't just use sbrk for that
purpose because we also want to free memory to the system.) For that goal,
emmalloc seems suitable as it is compact (we don't need it to be super-fast; this is
the system allocator that will be called rarely, compared to the fast one before it).
And for emmalloc to be used like that we need this PR so that we can build
emmalloc alongside another allocator (that other allocator will define malloc etc.
itself).

Helps #18369

sbc100

lgtm % test naming/nits

sbc100 · 2023-10-19T02:47:19Z

test/other/test_emmalloc_in_addition.c

+  // change in usage.
+  assert(emmalloc_dynamic_heap_size() == initial);
+#else
+  // malloc == emmalloc_malloc, so emmalloc will report additional usage (of the


You could maybe simplify this test by simply asserting that malloc == emmalloc_malloc as the command says? The you wouldn't need the comment to describe the code as it would more self explanatory?

By taking the address, you mean? I wasn't sure that is entirely reliable in general. In particular I'd worry about there being some kind of trampoline but it still going to the same target. (With aliases that wouldn't happen, but we might refactor to use another method.)

If we use another method I think its fine to update this test at that point... but that other method would seem like a bad idea since it would incur a cost.

I think there would be a risk that we could forget to update this test... it would be a needle in the haystack of the testsuite, that is, we'd need to somehow realize it needs to be updated then. Most likely we'd forget and it could start to pass incorrectly.

But surely the tests would fail because the assert "malloc == emmalloc" would fail right away, no?

In other words, I think it's safer to test this by checking the functionality - that emmalloc actually runs, or does not - and not an indirect property that is connected to that atm, if that makes sense.

Fair enough. This lgtm.

I realize you're right that malloc == emmalloc would fail one way at least, if we check both ways (as this PR currently does, so one path checks == and one !=). So my argument isn't as strong as I thought. Still, I think this is a pretty simple way to check the underlying functionality so I do feel it is safer.

I tried to add emmalloc == malloc testing as you suggested, because I thought it would be good to have both, but actually that fails atm, it turns out! We do have a trampoline:

emscripten/system/lib/emmalloc.c

Lines 836 to 839 in 4608c2e

void * EMMALLOC_EXPORT malloc(size_t size)

{

return emmalloc_malloc(size);

}

So in a debug build the two are obviously different. It turns out that even in a release build they are, because LLVM allocates two function pointer addresses, one for each. Optimizations (inlining + merging similar functions) end up making them identical, but there are still two function pointers, and the table ends up with two slots with the same function name in it...

It might be worth using an alias rather than a trampoline here, but I'm not sure if there was a reason not to do that. Anyhow, let's leave that separate from this PR, so I'll land this.

I can't get emmalloc == malloc testing to work in a followup.

I fixed the code to not have trampolines, and the function pointers are identical, and it works in a debug build, but the LLVM optimizer assumes they are not equal all the time. I seem to remember something about that being undefined behavior, that in some architectures function pointers behave very oddly... so I guess it's that.

sbc100 · 2023-10-19T02:47:26Z

test/test_other.py

+    # emmalloc == malloc.
+    'with_std_exp': (['-DTEST_EMMALLOC_IS_MALLOC'],),
+  })
+  def test_emmalloc_in_addition(self, args):


I don't love the name of this test (or the NO_STD_EXPORTS) but I'm struggling to come up with anything better.

I'm not sure you need the with_std_exp variant of this test since that is the default and tested elsewhere. Can we simplify this test by just verifying that it works as expected in the "NO_EXPORT" case?

Maybe test_emmalloc_explicit or test_emmalloc_standalone? I don't really love them either.

I considered "standalone", but I felt it is too similar to "standalone wasm" and so it's confusing. I could rename to "explicit" but I think "in addition" is slightly more clear?

I agree with_std_exp is tested elsewhere, but I like that testing both here in relation to that one flag shows the flag exactly controls that behavior. So if we ever get it wrong it won't be a random test that fails, but here. I'd also worry about us removing/modifying the other tests, though that seems like likely.

How about "alongside" instead of "in addition" or "explicit"? This does test that we can have emmalloc alongside another malloc at the same time.

sbc100 · 2023-10-19T18:24:32Z

system/lib/emmalloc.c


+#ifndef EMMALLOC_NO_STD_EXPORTS
 void * EMMALLOC_EXPORT malloc(size_t size)
 {
  return emmalloc_malloc(size);
 }


BTW, do you know why we are not using aliases here? Seems like it would be code size and performance win to not go through this wrapper. Perhaps we should make that change as a separate change?

Ah, I just saw this comment now after commenting above on this. I'm not sure why we trampoline here, and as I said there, optimizations remove 99% of the overhead but we do keep a table slot for them, so yeah, this might be worth improving.

sbc100 · 2023-10-19T18:25:49Z

test/test_other.py

+    # build emmalloc using -DEMMALLOC_NO_STD_EXPORTS it will not export malloc
+    # etc., and only provide the emmalloc_malloc etc. family of functions that
+    # we can use.
+    emmalloc = path_from_root('system', 'lib', 'emmalloc.c')


When you build emmalloc in addition to malloc here.. how does it work? How does emmalloc get it underlying memory? Are they both able to grab memory from the sbrk region somehow?

Both use sbrk to allocate. Neither frees anything to sbrk, so things work out.

(This won't be an issue for the intended use of this, though, which is that the second allocator calls emmalloc which calls sbrk.)

Rather than define malloc as a small function that calls emmalloc_malloc, just make it an alias, etc. In an optimized build the small functions get inlined away, but we can save that work, and also it can have downsides (e.g. they have different function pointers, which aliases do not, see discussion in #20487). For that to work, we need to make the aliases weak, to match the fact that the trampolines were weak.

kripken added 11 commits September 21, 2023 13:45

start

988dc95

avoid

dbc03c1

Merge remote-tracking branch 'origin/main' into emmalloc_options

5137356

work

8fec8dd

Merge remote-tracking branch 'origin/main' into emmalloc_options

d7013e0

work

a5b17a4

work

c33fe4a

work

de0c5db

work

9667e10

test

2d29660

Bettr

54500c9

kripken requested a review from sbc100 October 18, 2023 23:48

clean

34f67aa

sbc100 approved these changes Oct 19, 2023

View reviewed changes

sbc100 reviewed Oct 19, 2023

View reviewed changes

kripken merged commit 612d382 into main Oct 19, 2023
2 checks passed

kripken deleted the emmalloc_options branch October 19, 2023 19:38

kripken mentioned this pull request Oct 19, 2023

Avoid trampolines in emmalloc #20496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

emmalloc: Add an option to not define the standard exports #20487

emmalloc: Add an option to not define the standard exports #20487

kripken commented Oct 18, 2023

sbc100 left a comment

sbc100 Oct 19, 2023

kripken Oct 19, 2023

sbc100 Oct 19, 2023

kripken Oct 19, 2023

sbc100 Oct 19, 2023

kripken Oct 19, 2023

sbc100 Oct 19, 2023

kripken Oct 19, 2023

kripken Oct 19, 2023

kripken Oct 19, 2023

sbc100 Oct 19, 2023

sbc100 Oct 19, 2023

kripken Oct 19, 2023 •

edited

Loading

kripken Oct 19, 2023

sbc100 Oct 19, 2023

kripken Oct 19, 2023

sbc100 Oct 19, 2023

kripken Oct 19, 2023

	void * EMMALLOC_EXPORT malloc(size_t size)
	{
	return emmalloc_malloc(size);
	}

emmalloc: Add an option to not define the standard exports #20487

emmalloc: Add an option to not define the standard exports #20487

Conversation

kripken commented Oct 18, 2023

sbc100 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kripken Oct 19, 2023 • 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

kripken Oct 19, 2023 •

edited

Loading