Starlark: @StarlarkMethod.trustReturnsValid #13012
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jin
added
the
team-Starlark-Interpreter
facebook-github-bot
pushed a commit
to facebook/buck
that referenced
this pull request
Mar 10, 2021
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
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()
```
sgowroji
added
the
awaiting-review
|
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
removed
the
awaiting-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.javabuiltins with this annotation