-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Starlark: @StarlarkMethod.trustReturnsValid #13012
base: master
Are you sure you want to change the base?
Conversation
Summary: When returning from native functions, Starlark by default checks that returned value is a valid Starlark object (i. e. `String`, `Boolean` or implements `StarlarkValue`). That check is relatively expensive for cheap calls like `dict.get`. `StarlarkMethod.trustReturnsValid` when used with native function disables this check. This diff will likely not produce noticeable speedup, but the check is visible in profiler. Upstream PR: bazelbuild/bazel#13012 Reviewed By: mzlee fbshipit-source-id: 29e146503a0653a5629dc526c76d64e6f5e5b2f7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
When this parameter is specified for method, Starlark interpreter assumes function returns a valid Starlark value. When method is declared to return `Object`, Starlark interpreter by default checks that object is a valid Starlark value, which 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() ```
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. |
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:
net.starlark.java
builtins with this annotation