Skip to content

Commit

Permalink
wasm: misc optimizations (less locals, blocks, interning strings and …
Browse files Browse the repository at this point in the history
…booleans) (open-policy-agent#3179)

* wasm: introduce OPA_STRING_INTERNED for interned strings

opa_value_type will report these as OPA_STRING, so special behaviour
should use node->type to discern OPA_STRING/OPA_STRING_INTERNED:

- shallow copies don't need to copy interned strings
- interned strings aren't free()'ed

* wasm: pass constants along, compile them accordingly
* wasm/src: switch to stdbool.h's bool

I'm not aware of any strong reason not to, it seems to be what's commonly
advised, and the memory use of this type is smaller.

* wasm: intern opa_boolean

The heap allocs for these probably don't amount to much, but interning
them allows for shovelling them through the IR as constants. This lets
us shortcut the evaluation of (n)eq when both operands would be known
at compile-time (not likely). However, it also lets us safe a few more
locals, namely all the ones for MakeBooleanStmt.

* wasm: replace `opa_boolean()` by func returning interned bools

The new function will end up having this body:

    00b742 func[188] <opa_boolean>:
     00b743: 41 8a d0 03                | i32.const 59402
     00b747: 41 8c d0 03                | i32.const 59404
     00b74b: 20 00                      | local.get 0
     00b74d: 1b                         | select
     00b74e: 0b                         | end

Where the addresses correspond to our interned boolean `opa_value *`.

The previous implementation, should anyone need it, is still available
as `opa_boolean_allocated`. It's used in tests, too, where we do not
have the `opa_boolean()` emitted by our Wasm compiler.

* wasm: br_if/br optimizations for constants
* wasm: remove AssignBooleanStmt and opa_value_boolean_set

This could be trouble for our interned opa_boolean_t's, but it's not used.
So, let's just get rid of it.

* wasm: avoid some blocks where possible

Due to how the planner plans functions, any partial rule defining a
set or an object would have a block like this:

    block
      call 208 <opa_object>
      local.set 2
    end

With this change, those will no longer be wrapped.

It's not a big deal, neither in what it gets us, nor in what it takes
to apply the optimization.

* wasm: add one-branched if, use in memoization
* wasm: de-block internal calls

I've been comparing our instructions to what wasm-opt does to them, and
this seems like a reasonable change.

Signed-off-by: Stephan Renatus <[email protected]>
  • Loading branch information
srenatus authored Mar 3, 2021
1 parent dc820b5 commit bd5c572
Show file tree
Hide file tree
Showing 20 changed files with 668 additions and 582 deletions.
21 changes: 21 additions & 0 deletions internal/compiler/wasm/optimizations.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"time"

"github.com/open-policy-agent/opa/internal/wasm/encoding"
"github.com/open-policy-agent/opa/internal/wasm/instruction"
)

const warning = `---------------------------------------------------------------
Expand Down Expand Up @@ -80,3 +81,23 @@ func woptFound() bool {
_, err := exec.LookPath("wasm-opt")
return err == nil
}

// NOTE(sr): Yes, there are more control instructions than these two,
// but we haven't made use of them yet. So this function only checks
// for the control instructions we're possibly emitting, and which are
// relevant for block nesting.
func withControlInstr(is []instruction.Instruction) bool {
for _, i := range is {
switch i := i.(type) {
case instruction.Br, instruction.BrIf:
return true
case instruction.StructuredInstruction:
// NOTE(sr): We could attempt to further flatten the nested blocks
// here, but I believe we'd then have to correct block labels.
if withControlInstr(i.Instructions()) {
return true
}
}
}
return false
}
Loading

0 comments on commit bd5c572

Please sign in to comment.