Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exposing GC in binaryen.js APIs #5921

Open
jayphelps opened this issue Sep 7, 2023 · 12 comments
Open

Exposing GC in binaryen.js APIs #5921

jayphelps opened this issue Sep 7, 2023 · 12 comments

Comments

@jayphelps
Copy link
Contributor

I have a compiler written in TypeScript to aid in potential community maintainability that uses binaryen.js. It's going to rely on the GC instructions, but since historically they were still very in flux I had implemented things using a simple bump allocator instead while I wait for stability. Now that the GC proposal is in implementation phase I wanted to try and adopt it. I was happy to find that it seems like Binaryen has good support for them, but it isn't yet fully exposed via the JS API. Is that just a matter of no one seemingly needing it yet, or did you all want to hold off still?

@kripken
Copy link
Member

kripken commented Sep 7, 2023

No reason to hold off at this time: the GC spec is now very stable, as is Binaryen's implementation of it. So missing parts of the JS API are just that no one has gotten around to them yet, but it would be great to fill those out!

@jayphelps
Copy link
Contributor Author

@kripken Roger that. I discovered I can access them via the binaryen namespace directly e.g. binaryen._TypeBuilderCreate(1) so I'm going to try and get that working with my code first.

@jayphelps
Copy link
Contributor Author

Nice.

image

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Dec 4, 2023

@jayphelps I'm facing the exact same situation, but struggling to retrieve a usable type built using TypeBuilder.
Notably, how do you call _TypeBuilderBuildAndDispose ?
Can you share a working example for structs and arrays ?

@jayphelps
Copy link
Contributor Author

jayphelps commented Dec 4, 2023

@ericvergnaud Because the APIs are not yet exposed/wrapped in the clean JS API you have to do things a bit more manually, like allocating/deallocating space in binaryen's memory space for arguments. Here's an example, though it may have mistakes: (anyone is free to correct my mistakes, especially use-after-frees)

function allocU32Array(u32s: number[]): number {
  const { length } = u32s;
  const ptr = binaryen._malloc(length << 2);
  let offset = ptr;
  for (let i = 0; i < length; i++) {
    const value = u32s[i];
    binaryen.__i32_store(offset, value);
    offset += 4;
  }
  return ptr;
}

const tempStructIndex = 0;
const typeBuilder = binaryen._TypeBuilderCreate(1);
// I always use temps so that I can potentially create recursive types.
const tempStructHeapType = binaryen._TypeBuilderGetTempHeapType(typeBuilder, tempStructIndex);

const fieldTypes = [binaryen.i32];
const cFieldTypes = allocU32Array(fieldTypes);
const cFieldPackedTypes = allocU32Array(fieldTypes.map(() => binaryen._BinaryenPackedTypeNotPacked()));
const cFieldMutables = allocU32Array(fieldTypes.map(() => 0));
binaryen._TypeBuilderSetStructType(
  typeBuilder,
  tempStructIndex,
  cFieldTypes,
  cFieldPackedTypes,
  cFieldMutables,
  fieldTypes.length
);
binaryen._free(cFieldTypes);
binaryen._free(cFieldPackedTypes);
binaryen._free(cFieldMutables);

const size = binaryen._TypeBuilderGetSize(typeBuilder);
const out = binaryen._malloc(Math.max(4 * size, 8));
if (!binaryen._TypeBuilderBuildAndDispose(typeBuilder, out, out, out + 4)) {
  binaryen._free(out);
  throw new Error('_TypeBuilderBuildAndDispose failed');
}
// const structHeapType = binaryen.__i32_load(out + 4 * tempStructIndex);
// const structBinaryenType = binaryen._BinaryenTypeFromHeapType(structHeapType, false);
// const signatureHeapType = binaryen.__i32_load(out + 4 * tempSignatureIndex);
binaryen._free(out);

const structNewArgs = allocU32Array([this.module.i32.const(1337)]);
const structNew = binaryen._BinaryenStructNew(this.module, structNewArgs, 1, tempStructHeapType);
binaryen._free(structNewArgs);

module.addFunction(
  '_start',
  binaryen.createType([]),
  binaryen.i32,
  [],
  binaryen._BinaryenStructGet(this.module, 0, structNew, binaryen.i32, false)
);

module.addFunctionExport('_start', '_start');

I used AssemblyScript as a loose reference when I got stuck e.g. https://github.com/AssemblyScript/assemblyscript/blob/1e0466ef94fa5cacd0984e4f31a0087de51538a8/src/module.ts#L3689

I'm still unclear on when it's safe to free things, whether you have to free temp structs, etc. In some cases I'm just letting memory leak to be safe, since the compiler is short lived.

@ericvergnaud
Copy link
Contributor

@jayphelps Thanks for this. I thought I was missing something simple, the above makes sense when compared to c-api-kitchen-sink.c, but I didn't want to move that complexity up to the binaryen client..
I guess then it's worth submitting a PR to binaryen, which has access to stackAlloc and other helper methods that are erased at link time. I'll take care of that.

@ericvergnaud
Copy link
Contributor

@jayphelps see #6175
Feel free to comment

@samestep
Copy link

samestep commented Feb 6, 2024

@kripken I've run into this same issue; what's the best way to help contribute here? Should I try to help out with #6192?

@kripken
Copy link
Member

kripken commented Feb 6, 2024

I am actually not certain whether #6192 and #6225 have a dependency between them (if there is, then the latter needs to be figured out first). @ericvergnaud ?

@ericvergnaud
Copy link
Contributor

@kripken #6192 is definitely about that i.e. the initiative was triggered by the discrepancies between the C, the JS and the TS APIs, see #6145.

@samestep you probably want to check #6225. If the moves forward, I will welcome your help 😃

@samestep
Copy link

samestep commented Feb 7, 2024

@ericvergnaud thanks for the link! Yeah hopefully #6225 is resolved soon to allow #6192 to move forward.

@kripken the example from @jayphelps above is forced to use heap _malloc and _free because, while all the necessary TypeBuilder functions are indeed exposed in the version_116 release on npm, the stackSave, stackAlloc, allocateUTF8OnStack, and stackRestore functions are not available. In the short term, until #6225 is resolved to hopefully keep the JS/TS API more up to date going forward, would it be OK if I open a PR which just exposes those low-level stack functions in the JS API? That way, I'd be unblocked for my own usage of Binaryen because I can just use those stack functions directly, and those would only need to be available in the nightlies; hopefully the JS API would be updated by version_117, for instance, and those stack functions could then go away if you'd rather not have them exposed.

@kripken
Copy link
Member

kripken commented Feb 8, 2024

@samestep Yes, opening small reasonable intermediate PRs for now sounds fine. And I don't see a problem with exposing those stack APIs specifically in the long term (in a project the size of Binaryen, doing so has very small cost).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants