bazel icon indicating copy to clipboard operation
bazel copied to clipboard

Starlark: @StarlarkMethod.trustReturnsValid

Open stepancheg opened this issue 4 years ago • 1 comments
trafficstars

When @StarlarkMethod(trustReturnsValid = true) is specified, the interpreter does not check the method returns a valid Starlark object (which is does by default unless return type is statically known to be valid).

Object validity check is quite expensive.

Speed up is 4% for this test:

def test():
  for i in range(10):
    print(i)
    d = {"a": range(10), "b": {}, "c": 4, "d": 5, "e": (), "f": True, "g": []}
    for _ in range(1000000):
      d.get("a")
      d.get("b")
      d.get("c")
      d.get("d")
      d.get("e")
      d.get("f")
      d.get("g")

test()
  • First commit adds an annotation parameter and handling of this parameter
  • Second commit annotates net.starlark.java builtins with this annotation

stepancheg avatar Feb 13 '21 05:02 stepancheg

I'm on the fence about this one -- whether the 4% speedup is worth the extra annotation API. I don't love the idea of subverting a validity check with a "trust me, it's valid" flag, because that suggests we shouldn't be validating in the first place.

(Mostly I just wish Java would let us declare a type subsuming {StarlarkValue | String | Boolean} without paying the cost of a tagged union wrapper object.)

I'll punt on this now until tetromino is back in office (January) to discuss.

brandjon avatar Dec 19 '22 15:12 brandjon