java_stub_template to use argument file instead of classpath jar #1410

ittaiz · 2022-07-14T12:10:54Z

Description

java_stub_template to use argument file instead of classpath jar
This shaves 1s-1.5s when classpath is long.
Requires java > 8 so to properly add it in we'll probably have a toggle or use the toolchain or something
bazelbuild/rules_java#331

Motivation

performance

liucijus

@ittaiz I think having an attribute as a toggle on the scala toolchain is the easiest way

…ves 1s-1.5s, works java>8)

ittaiz · 2022-11-24T20:46:49Z

@liucijus we're good to go. Build is green.
One thing I really don't like about the current test is that it doesn't really test anything.
At maximum it tests there is such an attribute in the toolchain.
It doesn't check how it's used.
The main reason is because I'm not sure how to write non-flaky tests that compare perf.
Also the perf is felt when the classpath is huge so that is another problem.
I thought about maybe grepping through the bash script and see if it has True or False (after the substitutions that happen in starlark) but that feels wrong.

WDYT?

ittaiz · 2022-11-24T20:51:50Z

Btw, I think there's something flaky
https://buildkite.com/bazel/rules-scala-scala/builds/3951#0184ab62-2028-4d07-8a2f-d2048ed3a80f:


2022/11/24 20:48:36 Using unreleased version at commit 6d07ee5927283ecdab18822cc5a1a8a9a64d4074

Loading: 

Loading: 0 packages loaded

INFO: Build option --extra_toolchains has changed, discarding analysis cache.

Analyzing: target //test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider (0 packages loaded, 0 targets configured)

INFO: Analyzed target //test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider (0 packages loaded, 894 targets configured).

INFO: Found 1 target...

[0 / 2] [Prepa] BazelWorkspaceStatusAction stable-status.txt

ERROR: /workdir/test_expect_failure/missing_direct_deps/internal_deps/BUILD:117:14: scala @//test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider failed: (Exit 1): scalac failed: error executing command (from target //test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider) bazel-out/k8-opt-exec-2B5CBBC6/bin/src/java/io/bazel/rulesscala/scalac/scalac ... (remaining 1 argument skipped)

error: Target 'Unknown label of file bazel-out/k8-fastbuild/bin/test_expect_failure/missing_direct_deps/internal_deps/transitive_dependency_without_manifest.jar which came from @//test_expect_failure/missing_direct_deps/internal_deps:unstamped_direct_java_provider_dependency' is used but isn't explicitly declared, please add it to the deps.

You can use the following buildozer command:

buildozer 'add deps Unknown label of file bazel-out/k8-fastbuild/bin/test_expect_failure/missing_direct_deps/internal_deps/transitive_dependency_without_manifest.jar which came from @//test_expect_failure/missing_direct_deps/internal_deps:unstamped_direct_java_provider_dependency' //test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider

one error found

Build failed

java.lang.RuntimeException: Build failed

	at io.bazel.rulesscala.scalac.ScalacWorker.compileScalaSources(ScalacWorker.java:290)

	at io.bazel.rulesscala.scalac.ScalacWorker.work(ScalacWorker.java:61)

	at io.bazel.rulesscala.worker.Worker.persistentWorkerMain(Worker.java:86)

	at io.bazel.rulesscala.worker.Worker.workerMain(Worker.java:39)

	at io.bazel.rulesscala.scalac.ScalacWorker.main(ScalacWorker.java:25)

Target //test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider failed to build

Use --verbose_failures to see the command lines of failed build steps.

INFO: Elapsed time: 1.029s, Critical Path: 0.93s

INFO: 4 processes: 2 internal, 2 worker.

FAILED: Build did NOT complete successfully

'bazel build //test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider' should have logged "Unknown label of file .*transitive_dependency_without_manifest.jar which came from //test_expect_failure/missing_direct_deps/internal_deps:unstamped_direct_java_provider_dependency".

�[0;31m Test "test_scala_library_expect_better_failure_message_on_missing_transitive_dependency_labels_from_other_jvm_rules" failed �[0m (1 sec) �[0m

liucijus · 2022-11-25T08:31:36Z

scala/scala_toolchain.bzl

        ),
+        "use_argument_file_in_runner_for_improved_performance": attr.bool(
+            default = False,
+            doc = "Changes java binaries scripts (including tests) to use argument files and not classpath jars to improve performance, requires java > 8",


Did you mean JDK, with java > 8?

I actually meant the JRE (e.g. the jvm).
Do you want me to change the phrasing? I can also write JDK though it's the JRE part of it that we care about. I don't remember if in the current state of affairs in bazel these concepts are bundled or not

liucijus · 2022-11-25T08:39:46Z

@liucijus we're good to go. Build is green. One thing I really don't like about the current test is that it doesn't really test anything. At maximum it tests there is such an attribute in the toolchain. It doesn't check how it's used. The main reason is because I'm not sure how to write non-flaky tests that compare perf. Also the perf is felt when the classpath is huge so that is another problem. I thought about maybe grepping through the bash script and see if it has True or False (after the substitutions that happen in starlark) but that feels wrong.

WDYT?

@ittaiz I don't think you should test perf with such tests. The goal of such test should be to understand if everything wires up correctly and does correct build. I don't think we have any easy way to do perf regression here. So we have to trust that you've done your own measurements.

liucijus

In general LGTM, please shorten the name of the attribute

liucijus · 2022-11-25T08:33:50Z

scala/scala_toolchain.bzl

            default = True,
            doc = "Enable writing of statsfile",
        ),
+        "use_argument_file_in_runner_for_improved_performance": attr.bool(


I think shorter name is better: "use_argument_file_in_runner". Reasoning is already described in doc.

👍🏽 will change

liucijus · 2022-11-25T08:46:09Z

Btw, I think there's something flaky https://buildkite.com/bazel/rules-scala-scala/builds/3951#0184ab62-2028-4d07-8a2f-d2048ed3a80f:


2022/11/24 20:48:36 Using unreleased version at commit 6d07ee5927283ecdab18822cc5a1a8a9a64d4074

Loading: 

Loading: 0 packages loaded

INFO: Build option --extra_toolchains has changed, discarding analysis cache.

Analyzing: target //test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider (0 packages loaded, 0 targets configured)

INFO: Analyzed target //test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider (0 packages loaded, 894 targets configured).

INFO: Found 1 target...

[0 / 2] [Prepa] BazelWorkspaceStatusAction stable-status.txt

ERROR: /workdir/test_expect_failure/missing_direct_deps/internal_deps/BUILD:117:14: scala @//test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider failed: (Exit 1): scalac failed: error executing command (from target //test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider) bazel-out/k8-opt-exec-2B5CBBC6/bin/src/java/io/bazel/rulesscala/scalac/scalac ... (remaining 1 argument skipped)

error: Target 'Unknown label of file bazel-out/k8-fastbuild/bin/test_expect_failure/missing_direct_deps/internal_deps/transitive_dependency_without_manifest.jar which came from @//test_expect_failure/missing_direct_deps/internal_deps:unstamped_direct_java_provider_dependency' is used but isn't explicitly declared, please add it to the deps.

You can use the following buildozer command:

buildozer 'add deps Unknown label of file bazel-out/k8-fastbuild/bin/test_expect_failure/missing_direct_deps/internal_deps/transitive_dependency_without_manifest.jar which came from @//test_expect_failure/missing_direct_deps/internal_deps:unstamped_direct_java_provider_dependency' //test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider

one error found

Build failed

java.lang.RuntimeException: Build failed

	at io.bazel.rulesscala.scalac.ScalacWorker.compileScalaSources(ScalacWorker.java:290)

	at io.bazel.rulesscala.scalac.ScalacWorker.work(ScalacWorker.java:61)

	at io.bazel.rulesscala.worker.Worker.persistentWorkerMain(Worker.java:86)

	at io.bazel.rulesscala.worker.Worker.workerMain(Worker.java:39)

	at io.bazel.rulesscala.scalac.ScalacWorker.main(ScalacWorker.java:25)

Target //test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider failed to build

Use --verbose_failures to see the command lines of failed build steps.

INFO: Elapsed time: 1.029s, Critical Path: 0.93s

INFO: 4 processes: 2 internal, 2 worker.

FAILED: Build did NOT complete successfully

'bazel build //test_expect_failure/missing_direct_deps/internal_deps:unstamped_jar_dependent_on_some_java_provider' should have logged "Unknown label of file .*transitive_dependency_without_manifest.jar which came from //test_expect_failure/missing_direct_deps/internal_deps:unstamped_direct_java_provider_dependency".

�[0;31m Test "test_scala_library_expect_better_failure_message_on_missing_transitive_dependency_labels_from_other_jvm_rules" failed �[0m (1 sec) �[0m

I think tests got broken with Bazel 6 changes in backwards incompatible way.

ittaiz · 2022-11-25T09:25:57Z

Re perf and test- my point is that if you would keep the attribute on the toolchain but stop using it and also of course not pass it to the binary runner script the test still passes.
I can grep through the script to see if the value (True/False) is passed to the script but that test is more brittle and white box.
Wdyt?

liucijus · 2022-11-25T09:31:05Z

Re perf and test- my point is that if you would keep the attribute on the toolchain but stop using it and also of course not pass it to the binary runner script the test still passes. I can grep through the script to see if the value (True/False) is passed to the script but that test is more brittle and white box. Wdyt?

I think grepping for the value is useful check

ittaiz · 2022-11-25T10:01:39Z

I think grepping for the value is useful check

I'll change the test and assert on that

ittaiz · 2022-11-25T13:20:39Z

@liucijus done

ittaiz · 2022-11-25T13:42:04Z

I think the macos build is stuck for some reason. Maybe buildkite related. Running for 20m.
I tried canceling it from the UI but that didn't work.
Might need to timeout and then rerun.
All other builds passed

ittaiz · 2022-11-25T13:42:44Z

@liucijus if you have a way to maybe stop and start the build that would be great.

ittaiz · 2022-11-25T16:12:49Z

pushed another commit since that build is stuck

simuons · 2022-11-29T14:44:18Z

test/shell/test_scala_binary.sh

+test_scala_binary_allows_opt_in_to_use_of_argument_file_in_runner_for_improved_performance() {
+
+  bazel run --extra_toolchains="//test/toolchains:use_argument_file_in_runner" //test/src/main/scala/scalarules/test/large_classpath:largeClasspath
+  grep "\"True\" == \"True\"" bazel-bin/test/src/main/scala/scalarules/test/large_classpath/largeClasspath


Hi @ittaiz, overall looks good to me. I just find this grep pattern non intuitive. Maybe template could render something like test_runner_classpath_mode="argsfile"|"manifest" do an if on that variable and then grep for test_runner_classpath_mode="argsfile" in test? wdyt?

sure. I can do that. I also didn't like the grep. Vaidas wanted the use_argument_file thing and I went with it all the way. Will do the change.
To make sure we're aligned.
The new greps will be:

"argsfile" == "argsfile"

"manifest" == "argsfile"
Right?

Correct. That would be clearer. Thanks.

@simuons PTAL

ittaiz · 2022-11-30T09:11:11Z

./test_reproducibility.sh is running for 2h. Probably flaky.

liucijus · 2022-11-30T09:28:21Z

Thanks, @ittaiz!

ittaiz · 2022-11-30T09:36:11Z

😊 sure thing. Thank you both for your review!

ittaiz · 2022-11-30T09:36:31Z

Oh, I just realized I didn't update the docs :(

ittaiz requested review from liucijus and simuons as code owners July 14, 2022 12:10

liucijus requested changes Nov 21, 2022

View reviewed changes

java_stub_template to use argument file instead of classpath jar (sha…

4f8e59e

…ves 1s-1.5s, works java>8)

ittaiz force-pushed the stub_template_use_arguemtn branch from 6d5eaa9 to c8046cb Compare November 24, 2022 18:29

ittaiz added 3 commits November 24, 2022 20:29

use toolchain

c8046cb

change test to use run

a5fe9c0

lint

4085810

liucijus reviewed Nov 25, 2022

View reviewed changes

liucijus requested changes Nov 25, 2022

View reviewed changes

rename attribute

d798ef0

test now greps to see passed value from toolchain

21aa7eb

ittaiz closed this Nov 25, 2022

ittaiz reopened this Nov 25, 2022

readded whitespace that was lost

23f5359

liucijus approved these changes Nov 29, 2022

View reviewed changes

simuons reviewed Nov 29, 2022

View reviewed changes

ittaiz added 2 commits November 30, 2022 08:51

change variable name to clarify assertion

cc4ca39

lint

3020573

simuons approved these changes Nov 30, 2022

View reviewed changes

liucijus merged commit dee33f4 into bazel-contrib:master Nov 30, 2022

Uh oh!

java_stub_template to use argument file instead of classpath jar #1410

java_stub_template to use argument file instead of classpath jar #1410

Uh oh!

Conversation

ittaiz commented Jul 14, 2022

Description

Motivation

Uh oh!

liucijus left a comment

Choose a reason for hiding this comment

Uh oh!

ittaiz commented Nov 24, 2022

Uh oh!

ittaiz commented Nov 24, 2022

Uh oh!

liucijus Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ittaiz Nov 25, 2022

Choose a reason for hiding this comment

Uh oh!

liucijus commented Nov 25, 2022

Uh oh!

liucijus left a comment

Choose a reason for hiding this comment

Uh oh!

liucijus Nov 25, 2022

Choose a reason for hiding this comment

Uh oh!

ittaiz Nov 25, 2022

Choose a reason for hiding this comment

Uh oh!

liucijus commented Nov 25, 2022

Uh oh!

ittaiz commented Nov 25, 2022

Uh oh!

liucijus commented Nov 25, 2022

Uh oh!

ittaiz commented Nov 25, 2022

Uh oh!

ittaiz commented Nov 25, 2022

Uh oh!

ittaiz commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ittaiz commented Nov 25, 2022

Uh oh!

ittaiz commented Nov 25, 2022

Uh oh!

simuons Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

ittaiz Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

simuons Nov 29, 2022

Choose a reason for hiding this comment

Uh oh!

ittaiz Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

ittaiz commented Nov 30, 2022

Uh oh!

liucijus commented Nov 30, 2022

Uh oh!

ittaiz commented Nov 30, 2022

Uh oh!

ittaiz commented Nov 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

liucijus Nov 25, 2022 •

edited

Loading

ittaiz commented Nov 25, 2022 •

edited

Loading