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

wasm: dynamically dispatch data functions with "seen" ref pieces using call_indirect #3058

Merged
merged 6 commits into from
Jan 21, 2021

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Jan 11, 2021

  1. write an object corresponding to data paths into the module data
  2. initialize an opa_object_t from that using _initialize, called
    as the module's Start function
  3. write out CallDynamicStmts in IR when the ref is not all ground,
    but its vars have been seen
  4. compile those CallDynamicStmts to call_indirect invocations in
    WASM, preceded by a lookup using the path in the object prepared
    in (2.)
  5. if the lookup fails to come up with a result, the eval goes
    undefined

What it looks like:

With t.rego as

package t

p {
  data.foo[input.x].bar.p
}

and foo.rego as

package foo.a.bar

p = true

when building policy.wasm using opa build -t wasm -e t/p t.rego foo.rego,
the body of function g0.data.t.p will contain

i32.const 5
call $opa_array_with_cap
local.set $11
local.get $11
local.get $7
call $opa_array_append
local.get $11
local.get $8
call $opa_array_append
local.get $11
local.get $6
call $opa_array_append
local.get $11
local.get $9
call $opa_array_append
local.get $11
local.get $10
call $opa_array_append
local.get $0
local.get $1
local.get $11
call $opa_mapping_lookup
local.tee $12
i32.eqz
br_if $block
local.get $12
call_indirect $29 (type $1)
local.tee $13
i32.eqz
br_if $block

Where the array-related functions build an array of

["g0", "foo", input.x, "bar", "p"]

and pass that to opa_mapping_lookup to determine the element index to
pass to call_indirect. The lookup function returns 74 from the JSON
blob put into the data section,

(data $38 (i32.const 56485)
  "{\"g0\": {\"foo\": {\"a\": {\"bar\": {\"p\": 74}}}, \"t\": {\"p\": 75}}}")

iff input.x happens to be "a". Otherwise, it'll return 0, and the result
will end up being undefined.

Element 74 of the modules func table is, of course, $g0.data.foo.a.bar.p:

(elem $33 (i32.const 74)
  $g0.data.foo.a.bar.p $g0.data.t.p)

($33 is some id of that piece of function table, an artifact of the
wavm disassemble output.)


Fixes #2936.

@srenatus
Copy link
Contributor Author

ℹ️ Pushed my latest WIP; the planner changes are not correct yet.

💣 On my machine at least, I had to resort to using v8 for testing (make wasm-rego-test); our golang SDK explodes with this branch, in the same way it does for the jsonpatch tests.

@srenatus srenatus force-pushed the sr/wasm/call_indirect branch from 5be78e2 to 3276bb9 Compare January 13, 2021 14:22
@srenatus
Copy link
Contributor Author

✔️ Having _initialize called through the Start section mechanism seems to work fine. 😃

@srenatus srenatus force-pushed the sr/wasm/call_indirect branch 3 times, most recently from c85d8fd to b390f5b Compare January 19, 2021 15:05
@srenatus srenatus changed the title WIP: wasm: dynamically dispatch data functions using call_indirect wasm: dynamically dispatch data functions with "seen" ref pieces using call_indirect Jan 19, 2021
@srenatus srenatus force-pushed the sr/wasm/call_indirect branch 3 times, most recently from 978ec4d to 76920ca Compare January 19, 2021 15:35
@srenatus
Copy link
Contributor Author

srenatus commented Jan 19, 2021

Two more things to check:

  • there's an extra block we might not need
    • it really doesn't matter. we have the same with CallStmt.
  • there should be a test case involving with, just to be sure

Currently, the test cases don't really check that call_indirect was used; but they help (me) to poke through the WASM code, seeing if everything is in order.

@srenatus srenatus force-pushed the sr/wasm/call_indirect branch 4 times, most recently from 72cac08 to 74024e5 Compare January 20, 2021 15:22
@srenatus srenatus marked this pull request as ready for review January 20, 2021 15:22
@srenatus srenatus force-pushed the sr/wasm/call_indirect branch 2 times, most recently from fd68606 to 67535af Compare January 20, 2021 16:02
Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. The planner changes are quite clean! I've left a few comments.

wasm/src/value.c Outdated Show resolved Hide resolved
wasm/tests/test.c Outdated Show resolved Hide resolved
internal/planner/planner.go Show resolved Hide resolved
// on subtrees of virtual already lost parts of the path we've taken.
if index == 1 {
rulesets, stmts, locals, index, optimize := p.optimizeLookup(virtual, ref)
if optimize {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could imagine it being useful to have the planner emit debug messages like compile/compile.go does for some of the optimizations it performs. Otherwise it becomes tricky to understand why the optimization hasn't kicked in.

We now

1. write an object corresponding to data paths into the module data
2. initialize an opa_object_t from that using `_initialize`, called
   as the module's Start function
3. write out CallDynamicStmts in IR when the ref is not all ground,
   but its vars have been seen
4. compile those CallDynamicStmts to call_indirect invocations in
   WASM, preceded by a lookup using the path in the object prepared
   in (2.)
5. if the lookup fails to come up with a result, the eval goes
   undefined

Stuff that's left out:

- memoization (see comment in internal/compiler/wasm/wasm.go)

What it looks like:

With t.rego as

    package t

    p {
      data.foo[input.x].bar.p
    }

and foo.rego as

    package foo.a.bar

    p = true

when building policy.wasm using `opa build -t wasm -e t/p t.rego foo.rego`,
the body of function `g0.data.t.p` will contain

    i32.const 5
    call $opa_array_with_cap
    local.set $11
    local.get $11
    local.get $7
    call $opa_array_append
    local.get $11
    local.get $8
    call $opa_array_append
    local.get $11
    local.get $6
    call $opa_array_append
    local.get $11
    local.get $9
    call $opa_array_append
    local.get $11
    local.get $10
    call $opa_array_append
    local.get $0
    local.get $1
    local.get $11
    call $opa_mapping_lookup
    local.tee $12
    i32.eqz
    br_if $block
    local.get $12
    call_indirect $29 (type $1)
    local.tee $13
    i32.eqz
    br_if $block

Where the array-related functions build an array of

    ["g0", "foo", input.x, "bar", "p"]

and pass that to `opa_mapping_lookup` to determine the element index to
pass to `call_indirect`. The lookup function returns 74 from the JSON
blob put into the data section,

    (data $38 (i32.const 56485)
      "{\"g0\": {\"foo\": {\"a\": {\"bar\": {\"p\": 74}}}, \"t\": {\"p\": 75}}}")

iff input.x happens to be "a". Otherwise, it'll return 0, and the result
will end up being undefined.

Element 74 of the modules func table is, of course, $g0.data.foo.a.bar.p:

    (elem $33 (i32.const 74)
      $g0.data.foo.a.bar.p $g0.data.t.p)

($33 is some id of that piece of function table, an artifact of the
`wavm disassemble` output.)

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
- adds a data segment for mapping element indices (used with call_indirect)
  to function indices (as used with opa_memoize_{get,insert})
- emits mapping function elem -> func idx that uses that data segment
- wires up memoization lookup and insert in call_indirect code path

The added test case would cause `make wasm-rego-test` to fail like this
if memoization wasn't happening:

    ERROR 019_call_indirect_optimization.json: memoization: should have been memoized

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus srenatus force-pushed the sr/wasm/call_indirect branch 3 times, most recently from d8d0725 to 35d3f1b Compare January 21, 2021 15:47
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus srenatus force-pushed the sr/wasm/call_indirect branch from 35d3f1b to cf3442d Compare January 21, 2021 16:20
Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@srenatus srenatus merged commit 81a774a into open-policy-agent:master Jan 21, 2021
@srenatus srenatus deleted the sr/wasm/call_indirect branch January 21, 2021 17:17
srenatus added a commit that referenced this pull request Jan 25, 2021
…#3089)

In our previous optimization change, #3058, we've been falling back to the
old behaviour (with introduces cross-products) whenever our optimization
didn't seem to apply.

This PR extends that a little: when there are only leaves (matching our
optimization lookup) that do not have rules (i.e. empty packages), we decide
that nothing package-related is to be done at all. So, we'll go on looking at
base data only.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
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

Successfully merging this pull request may close these issues.

Planner generates cross-product given non-ground refs to packages
2 participants