wasm: clarify opa_eval args

[christophwille]

https://github.com/open-policy-agent/opa/blob/main/docs/content/wasm.md

str_addr opa_eval(addr, entrypoint_id, value_addr, str_addr, int32, addr, format)

The int32 is supposed to be?

Also a question on entrypoint_id: unless you call opa_eval_ctx_set_entrypoint the default is used for eval. So what is the "default value" for default entry point in this case?

[srenatus]

The int32 is supposed to be?

Sorry, those match the text, but the presentation there could surely be improved -- adding the types, we have:

entrypoint (entrypoint_id), address of data in memory (value_addr), address (str_addr) and length (int32) of input JSON string in memory, heap address (addr) to use, and the output format (format)

The int32 is the length of the input string written to memory.

Does that help?

---

Also a question on entrypoint_id: unless you call opa_eval_ctx_set_entrypoint the default is used for eval. So what is the "default value" for default entry point in this case?

That'll be 0, the first entrypoint.

[christophwille]

I am getting somewhere but definitely doing something wrong in terms of what I need / no longer need to call...

https://github.com/christophwille/dotnet-opa-wasm/blob/0c1421e5b5d4450f0a678e2e27286d4ff2f72604/src/Opa.Wasm/OpaPolicy.cs#L164

(old Evaluate is the method above, it mirros the JS impl)

I am quite sure that something in the chain outlined via #3627 is wrong in my implementation because the unit test is failing (identical to Evaluate).

[srenatus]

I don't know how the C# wasm stuff works, but the idea is that you wouldn't even call opa_malloc to allocate space for the input json, you'd just write it at dataHeapPtr, and add the input length to dataHeapPtr, which is then passed to opa_eval. Also, you wouldn't need to parse it. All that now happens in opa_eval: https://github.com/open-policy-agent/opa/blob/v0.31.0/wasm/src/context.c#L27-L44

[christophwille]

A question on that code piece - there is a distinction in dumping between json (opa_json_dump) and value (opa_value_dump), however, on parsing it is always value parsing (opa_value_parse). Is this intentional?

[christophwille]

I modified as per your suggestions:

public string FastEvaluate(string json)
{
  _envMemory.WriteString(_store, _dataHeapPtr, json);

  int resultaddr = Policy_opa_eval(_dataAddr, _dataHeapPtr, json.Length, _dataHeapPtr + json.Length);

  return _envMemory.ReadNullTerminatedString(_store, resultaddr);
}

https://github.com/christophwille/dotnet-opa-wasm/blob/1c7fce613f9da3d9eaec1f3f5213b4fecffe165f/src/Opa.Wasm/OpaPolicy.cs#L164

This makes it work... but sure feels strange to modify the heap in this "rude" fashion. Btw, what should _dataHeapPtr be after the evalution? (so a new call to FastEvaluate is working properly - reset to original to overwrite what was there, or end of 1st JSON?)

[srenatus]

there is a distinction in dumping between json (opa_json_dump) and value (opa_value_dump), however, on parsing it is always value parsing (opa_value_parse). Is this intentional?

Yeah, so, any JSON is a valid "Value" format -- the distinction between the two is sets, which don't exist in JSON. So,

{ "foo": { "bar", "baz", "quz" } }

would be a valid "value" string, but not valid JSON. When reading the input, however, it's up to the caller to provide JSON or "value". For the output, however, we let the caller decide: if they can make sense of "value" (sets in json), then they can have that; if they cannot, they get the JSON variant -- sets become arrays.

This makes it work... but sure feels strange to modify the heap in this "rude" fashion.

😄 It's certainly a bit of a rough fast path, I grant you that. Personally, I think any low-level memory management done from the outside (messing with the heap ptr etc) is crude, but it's the way we've taken for now. Maybe some ABI 2 will happen that is both elegant, and works, and doesn't require a dozen VM calls...? Ideas are welcome.

what should _dataHeapPtr be after the evalution? [...] reset to original to overwrite what was there, or end of 1st JSON?

It should be be reset to original. If you put it at the end of the first JSON input, the next JSON input will be written after that, and the next after that, etc, and you'll eventually run out of memory on subsequent calls to FastEvaluate -- I had done that at some point while working on the new method. 😄

[christophwille]

ok, so my not modifying _dataHeapPtr is the correct approach then.

[christophwille]

I added benchmark results: christophwille/dotnet-opa-wasm#16 - note that this doesn't reuse the policy but create it new each time. So the general overhead eats up most gains.

[srenatus]

🎉 Thanks again for updating the SDK. I suppose this can be closed now? 🤔

[christophwille]

Yes, unless want to use it to track docs improvements from this.

[srenatus]

😄 fair enough. Let's do that.

[srenatus]

While I'd love a full-blown, useful-for-more-than-just-docs WITX definition here, I'll look into making a small improvement for clarifying the arguments... 😄

[srenatus]

@christophwille would be you be able to have a look at that PR? Your input would be appreciated! 😃