Skip to content

Commit

Permalink
starlark: Reduce allocations for find/indexof without end
Browse files Browse the repository at this point in the history 
By using the overload of `String#indexOf` that accepts a starting position, the common case of Starlark string forward searches that extend to the end of the string no longer requires allocations.

Before:
```
benchmark                        ops     cpu/op    wall/op   steps/op   alloc/op
bench_find                  33554431      223ns      221ns          6       143B
bench_find_with_start       33554431      238ns      236ns          6       631B
```

After:
```
benchmark                        ops     cpu/op    wall/op   steps/op   alloc/op
bench_find                  33554431      221ns      219ns          6       143B
bench_find_with_start       33554431      210ns      210ns          6       143B
```

Closes #17892.

PiperOrigin-RevId: 521450801
Change-Id: I2849f9d5a577973d24ca3644136550d0e2a8a569
  • Loading branch information
@fmeum
fmeum authored and Copybara-Service committed Apr 3, 2023
1 parent 233ee29 commit 70c821a
Show file tree
Hide file tree
Showing 21 changed files with 120 additions and 102 deletions.
6 changes: 3 additions & 3 deletions scripts/packages/BUILD
View file
Expand Up @@ -6,8 +6,8 @@ filegroup(
name = "srcs",
srcs = glob(["**"]) + [
"//scripts/packages/debian:srcs",
"//scripts/packages/fedora:srcs",
"//scripts/packages/dmg:srcs",
"//scripts/packages/fedora:srcs",
],
visibility = [
"//scripts:__pkg__",
Expand All @@ -25,14 +25,14 @@ filegroup(
"//src/conditions:freebsd": [],
"//src/conditions:openbsd": [],
"//src/conditions:darwin": [
":generate-package-info",
":with-jdk/install.sh",
":without-jdk/install.sh",
":generate-package-info",
],
"//conditions:default": [
":generate-package-info",
":with-jdk/install.sh",
":without-jdk/install.sh",
":generate-package-info",
"//:bazel-distfile",
"//scripts/packages/debian:bazel-debian",
"//scripts/packages/debian:bazel-debian-src",
Expand Down
38 changes: 19 additions & 19 deletions src/BUILD
View file
Expand Up @@ -90,33 +90,33 @@ JAVA_TOOLS = [
srcs = JAVA_TOOLS + [
"BUILD.tools",
"MODULE.tools",
"//tools:embedded_tools_srcs",
"//src/conditions:embedded_tools",
"//src/main/cpp/util:embedded_tools",
"//src/main/native:embedded_tools",
"//src/main/protobuf:srcs",
"//src/tools/android/java/com/google/devtools/build/android:embedded_tools",
"//src/tools/launcher:srcs",
"//third_party:gpl-srcs",
"//third_party/def_parser:srcs",
"//third_party/grpc:embedded_tools_srcs",
"//third_party/grpc/bazel:embedded_tools_srcs",
"//third_party/ijar:embedded_zipper_sources",
"//third_party/ijar:zipper",
"//third_party/java/j2objc:embedded_tools_srcs",
"//third_party/py/abseil:srcs",
"//third_party/py/six:srcs",
"//src/conditions:embedded_tools",
"//src/tools/android/java/com/google/devtools/build/android:embedded_tools",
"//src/tools/launcher:srcs",
"//src/main/cpp/util:embedded_tools",
"//src/main/native:embedded_tools",
"//src/main/protobuf:srcs",
"//third_party/def_parser:srcs",
"//third_party/zlib:embedded_tools",
"//tools:embedded_tools_srcs",
] + select({
"//src/conditions:darwin": [
"//tools/osx:xcode_locator.m",
],
"//conditions:default": [],
}) + select({
"//src/conditions:windows": [
"//src/tools/launcher:launcher",
"//src/tools/launcher",
"//src/tools/launcher:launcher_maker",
"//third_party/def_parser:def_parser",
"//third_party/def_parser",
],
"//conditions:default": [],
}) +
Expand Down Expand Up @@ -339,7 +339,6 @@ filegroup(
"//src/java_tools/junitrunner/javatests/com/google/testing/coverage:srcs",
"//src/java_tools/singlejar:srcs",
"//src/main/cpp:srcs",
"//src/main/res:srcs",
"//src/main/java/com/google/devtools/build/docgen:srcs",
"//src/main/java/com/google/devtools/build/lib:srcs",
"//src/main/java/com/google/devtools/build/lib/includescanning:srcs",
Expand All @@ -349,16 +348,15 @@ filegroup(
"//src/main/java/com/google/devtools/build/skyframe:srcs",
"//src/main/java/com/google/devtools/common/options:srcs",
"//src/main/java/net/starlark/java/cmd:srcs",
"//src/main/java/net/starlark/java/spelling:srcs",
"//src/main/java/net/starlark/java/lib/json:srcs",
"//src/main/java/net/starlark/java/spelling:srcs",
"//src/main/native:srcs",
"//src/main/protobuf:srcs",
"//src/main/res:srcs",
"//src/main/starlark/builtins_bzl:srcs",
"//src/main/tools:srcs",
"//src/test/cpp:srcs",
"//src/test/gen:srcs",
"//src/test/res:srcs",
"//src/test/native/windows:srcs",
"//src/test/java/com/google/devtools/build/android:srcs",
"//src/test/java/com/google/devtools/build/docgen:srcs",
"//src/test/java/com/google/devtools/build/lib:srcs",
Expand All @@ -368,18 +366,20 @@ filegroup(
"//src/test/java/com/google/devtools/common/options:srcs",
"//src/test/java/net/starlark/java/eval:srcs",
"//src/test/java/net/starlark/java/spelling:srcs",
"//src/test/native/windows:srcs",
"//src/test/py/bazel:srcs",
"//src/test/res:srcs",
"//src/test/shell:srcs",
"//src/test/testdata/test_tls_certificate",
"//src/test/tools:srcs",
"//src/tools/android:srcs",
"//src/tools/android/java/com/google/devtools/build/android:srcs",
"//src/tools/execlog:srcs",
"//src/tools/workspacelog:srcs",
"//src/tools/launcher:srcs",
"//src/tools/starlark/java/com/google/devtools/starlark/common:srcs",
"//src/tools/singlejar:srcs",
"//src/tools/remote:srcs",
"//src/tools/singlejar:srcs",
"//src/tools/starlark/java/com/google/devtools/starlark/common:srcs",
"//src/tools/workspacelog:srcs",
"//tools/osx:srcs",
],
visibility = ["//:__pkg__"],
Expand Down Expand Up @@ -564,13 +564,13 @@ filegroup(
"@bazel_skylib//:WORKSPACE",
"@com_google_protobuf//:WORKSPACE",
"@remote_coverage_tools//:WORKSPACE",
"@remote_java_tools_darwin_x86_64_for_testing//:WORKSPACE",
"@remote_java_tools_darwin_arm64_for_testing//:WORKSPACE",
"@remote_java_tools_darwin_x86_64_for_testing//:WORKSPACE",
"@remote_java_tools_for_testing//:WORKSPACE",
"@remote_java_tools_linux_for_testing//:WORKSPACE",
"@remote_java_tools_test//:WORKSPACE",
"@remote_java_tools_test_darwin_x86_64//:WORKSPACE",
"@remote_java_tools_test_darwin_arm64//:WORKSPACE",
"@remote_java_tools_test_darwin_x86_64//:WORKSPACE",
"@remote_java_tools_test_linux//:WORKSPACE",
"@remote_java_tools_test_windows//:WORKSPACE",
"@remote_java_tools_windows_for_testing//:WORKSPACE",
Expand Down
2 changes: 1 addition & 1 deletion src/main/cpp/BUILD
View file
Expand Up @@ -92,9 +92,9 @@ cc_binary(
srcs = [
"blaze.cc",
"blaze.h",
"main.cc",
"server_process_info.cc",
"server_process_info.h",
"main.cc",
] + select({
"//src/conditions:windows": ["resources.o"],
"//conditions:default": [],
Expand Down
16 changes: 11 additions & 5 deletions src/main/java/net/starlark/java/eval/StringModule.java
View file
Expand Up @@ -526,14 +526,20 @@ public String title(String self) throws EvalException {
private static int stringFind(boolean forward, String self, String sub, Object start, Object end)
throws EvalException {
long indices = substringIndices(self, start, end);
// Unfortunately Java forces us to allocate here, even though
// String has a private indexOf method that accepts indices.
// Fortunately the common case is self[0:n].
String substr = self.substring(lo(indices), hi(indices));
int startpos = lo(indices);
int endpos = hi(indices);
// Unfortunately Java forces us to allocate here in the general case, even
// though String has a private indexOf method that accepts indices.
// The common cases of a search of the full string or a forward search with
// a custom start position do not require allocations.
if (forward && endpos == self.length()) {
return self.indexOf(sub, startpos);
}
String substr = self.substring(startpos, endpos);
int subpos = forward ? substr.indexOf(sub) : substr.lastIndexOf(sub);
return subpos < 0
? subpos //
: subpos + lo(indices);
: subpos + startpos;
}

private static final Pattern SPLIT_LINES_PATTERN =
Expand Down
2 changes: 1 addition & 1 deletion src/main/native/BUILD
View file
Expand Up @@ -24,8 +24,8 @@ filegroup(
name = "jni_os",
srcs = select({
"//src/conditions:darwin": [
"darwin/fsevents.cc",
"darwin/file_jni.cc",
"darwin/fsevents.cc",
"darwin/sleep_prevention_jni.cc",
"darwin/system_cpu_speed_monitor_jni.cc",
"darwin/system_disk_space_monitor_jni.cc",
Expand Down
2 changes: 1 addition & 1 deletion src/main/tools/BUILD
View file
Expand Up @@ -50,8 +50,8 @@ cc_binary(
deps = select({
"//src/conditions:windows": [],
"//conditions:default": [
":process-tools",
":logging",
":process-tools",
],
}),
)
Expand Down
2 changes: 1 addition & 1 deletion src/test/cpp/BUILD
View file
Expand Up @@ -15,8 +15,8 @@ cc_test(
"blaze_util_windows_test.cc",
],
"//conditions:default": [
"blaze_util_test.cc",
"blaze_util_posix_test.cc",
"blaze_util_test.cc",
],
}),
deps = [
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/google/devtools/build/android/BUILD
View file
Expand Up @@ -3,9 +3,9 @@ load("@rules_java//java:defs.bzl", "java_library", "java_test")
filegroup(
name = "srcs",
srcs = glob(["**"]) + [
"//src/test/java/com/google/devtools/build/android/idlclass:srcs",
"//src/test/java/com/google/devtools/build/android/desugar:srcs",
"//src/test/java/com/google/devtools/build/android/dexer:srcs",
"//src/test/java/com/google/devtools/build/android/idlclass:srcs",
"//src/test/java/com/google/devtools/build/android/junctions:srcs",
"//src/test/java/com/google/devtools/build/android/r8:srcs",
"//src/test/java/com/google/devtools/build/android/resources:srcs",
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/google/devtools/build/android/r8/BUILD
View file
Expand Up @@ -29,8 +29,8 @@ java_library(
"//conditions:default": ["NoAndroidSdkStubTest.java"],
}),
deps = [
"//src/test/java/com/google/devtools/build/lib/testutil:TestSuite",
"//src/main/java/com/google/devtools/common/options:options_internal",
"//src/test/java/com/google/devtools/build/lib/testutil:TestSuite",
"//src/tools/android/java/com/google/devtools/build/android/r8",
"//third_party:guava",
"//third_party:junit4",
Expand Down
16 changes: 8 additions & 8 deletions src/test/java/com/google/devtools/build/lib/BUILD
View file
Expand Up @@ -18,9 +18,9 @@ filegroup(
"//src/test/java/com/google/devtools/build/lib/bazel:srcs",
"//src/test/java/com/google/devtools/build/lib/bazel/google:srcs",
"//src/test/java/com/google/devtools/build/lib/blackbox:srcs",
"//src/test/java/com/google/devtools/build/lib/bugreport:srcs",
"//src/test/java/com/google/devtools/build/lib/buildeventservice:srcs",
"//src/test/java/com/google/devtools/build/lib/buildeventstream:srcs",
"//src/test/java/com/google/devtools/build/lib/bugreport:srcs",
"//src/test/java/com/google/devtools/build/lib/buildtool:srcs",
"//src/test/java/com/google/devtools/build/lib/cmdline:srcs",
"//src/test/java/com/google/devtools/build/lib/collect:srcs",
Expand All @@ -31,14 +31,14 @@ filegroup(
"//src/test/java/com/google/devtools/build/lib/generatedprojecttest:srcs",
"//src/test/java/com/google/devtools/build/lib/generatedprojecttest/util:srcs",
"//src/test/java/com/google/devtools/build/lib/graph:srcs",
"//src/test/java/com/google/devtools/build/lib/io:srcs",
"//src/test/java/com/google/devtools/build/lib/integration/util:srcs",
"//src/test/java/com/google/devtools/build/lib/io:srcs",
"//src/test/java/com/google/devtools/build/lib/metrics:srcs",
"//src/test/java/com/google/devtools/build/lib/outputfilter:srcs",
"//src/test/java/com/google/devtools/build/lib/packages:srcs",
"//src/test/java/com/google/devtools/build/lib/pkgcache:srcs",
"//src/test/java/com/google/devtools/build/lib/packages/metrics:srcs",
"//src/test/java/com/google/devtools/build/lib/packages/semantics:srcs",
"//src/test/java/com/google/devtools/build/lib/pkgcache:srcs",
"//src/test/java/com/google/devtools/build/lib/platform:srcs",
"//src/test/java/com/google/devtools/build/lib/platform/darwin:srcs",
"//src/test/java/com/google/devtools/build/lib/profiler:srcs",
Expand All @@ -50,20 +50,20 @@ filegroup(
"//src/test/java/com/google/devtools/build/lib/rules:srcs",
"//src/test/java/com/google/devtools/build/lib/sandbox:srcs",
"//src/test/java/com/google/devtools/build/lib/server:srcs",
"//src/test/java/com/google/devtools/build/lib/skyframe:srcs",
"//src/test/java/com/google/devtools/build/lib/skyframe/packages:srcs",
"//src/test/java/com/google/devtools/build/lib/skyframe/serialization:srcs",
"//src/test/java/com/google/devtools/build/lib/skyframe:srcs",
"//src/test/java/com/google/devtools/build/lib/standalone:srcs",
"//src/test/java/com/google/devtools/build/lib/starlark:srcs",
"//src/test/java/com/google/devtools/build/lib/starlarkdebug/server:srcs",
"//src/test/java/com/google/devtools/build/lib/supplier:srcs",
"//src/test/java/com/google/devtools/build/lib/versioning:srcs",
"//src/test/java/com/google/devtools/build/lib/vfs:srcs",
"//src/test/java/com/google/devtools/build/lib/testing/common:srcs",
"//src/test/java/com/google/devtools/build/lib/testutil:srcs",
"//src/test/java/com/google/devtools/build/lib/unix:srcs",
"//src/test/java/com/google/devtools/build/lib/unsafe:srcs",
"//src/test/java/com/google/devtools/build/lib/util:srcs",
"//src/test/java/com/google/devtools/build/lib/testing/common:srcs",
"//src/test/java/com/google/devtools/build/lib/testutil:srcs",
"//src/test/java/com/google/devtools/build/lib/versioning:srcs",
"//src/test/java/com/google/devtools/build/lib/vfs:srcs",
"//src/test/java/com/google/devtools/build/lib/view/cpp:srcs",
"//src/test/java/com/google/devtools/build/lib/view/java:srcs",
"//src/test/java/com/google/devtools/build/lib/view/util:srcs",
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/com/google/devtools/build/lib/blackbox/BUILD
View file
Expand Up @@ -39,10 +39,10 @@ filegroup(
name = "srcs",
testonly = 0,
srcs = glob(["**"]) + [
"//src/test/java/com/google/devtools/build/lib/blackbox/framework:srcs",
"//src/test/java/com/google/devtools/build/lib/blackbox/bazel:srcs",
"//src/test/java/com/google/devtools/build/lib/blackbox/tests:srcs",
"//src/test/java/com/google/devtools/build/lib/blackbox/framework:srcs",
"//src/test/java/com/google/devtools/build/lib/blackbox/junit:srcs",
"//src/test/java/com/google/devtools/build/lib/blackbox/tests:srcs",
],
visibility = ["//src/test/java/com/google/devtools/build/lib:__pkg__"],
)
6 changes: 3 additions & 3 deletions src/test/java/com/google/devtools/build/lib/blackbox/tests/workspace/BUILD
View file
Expand Up @@ -48,8 +48,8 @@ java_test(
deps = common_deps + [
"//src/main/java/com/google/devtools/build/lib/bazel/repository",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/test/java/com/google/devtools/build/lib/vfs/util",
"//src/test/java/com/google/devtools/build/lib/events:testutil",
"//src/test/java/com/google/devtools/build/lib/vfs/util",
],
)

Expand All @@ -66,8 +66,8 @@ java_test(
deps = common_deps + [
"//src/main/java/com/google/devtools/build/lib/bazel/repository",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/test/java/com/google/devtools/build/lib/vfs/util",
"//src/test/java/com/google/devtools/build/lib/events:testutil",
"//src/test/java/com/google/devtools/build/lib/vfs/util",
],
)

Expand All @@ -82,8 +82,8 @@ java_test(
deps = common_deps + [
"//src/main/java/com/google/devtools/build/lib/bazel/repository",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/test/java/com/google/devtools/build/lib/vfs/util",
"//src/test/java/com/google/devtools/build/lib/events:testutil",
"//src/test/java/com/google/devtools/build/lib/vfs/util",
],
)

Expand Down
2 changes: 1 addition & 1 deletion src/test/java/com/google/devtools/build/lib/remote/BUILD
View file
Expand Up @@ -10,8 +10,8 @@ filegroup(
testonly = 0,
srcs = glob(["**"]) + [
"//src/test/java/com/google/devtools/build/lib/remote/downloader:srcs",
"//src/test/java/com/google/devtools/build/lib/remote/http:srcs",
"//src/test/java/com/google/devtools/build/lib/remote/grpc:srcs",
"//src/test/java/com/google/devtools/build/lib/remote/http:srcs",
"//src/test/java/com/google/devtools/build/lib/remote/logging:srcs",
"//src/test/java/com/google/devtools/build/lib/remote/merkletree:srcs",
"//src/test/java/com/google/devtools/build/lib/remote/options:srcs",
Expand Down

0 comments on commit 70c821a

Please sign in to comment.