Improve performance of Java package parser by making it a Bazel worker #3686

keithl-stripe · 2022-06-14T20:23:03Z

Checklist

I have filed an issue about this change
and discussed potential changes with the maintainers.
I have received the approval from the maintainers to make this change.
This is not a stylistic, refactoring, or cleanup change.

Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.

Discussion thread for this change

Issue number: #3685

Description of this change

Makes the package parser tool (a Java CLI application) able to be run as a Bazel worker. Also modify the Starlark to enable worker mode.

Bencodes · 2022-06-14T21:00:47Z

aspect/intellij_info_impl.bzl

@@ -704,6 +712,10 @@ def build_java_package_manifest(ctx, target, source_files, suffix):
        arguments = [args],
        mnemonic = "JavaPackageManifest",
        progress_message = "Parsing java package strings for " + str(target.label),
+        execution_requirements = {


I think it would be great to also support multiplexed workers here.

@keithl-stripe is this something that makes sense from your perspective?

Sure, but for now that's a "nice to have" - I believe it would simply improve overall performance, but should not be a blocker for this PR.

Bencodes · 2022-07-06T21:25:47Z

@mai93 can you take a look at this?

jastice

LGTM. Any problems you see @mai93 ?

Comments from @keithl-stripe in chat:

AFAIK generally workers have some risks:

could add to memory footprint of a build, e.g accidental state or uncollected garbage, or simply the fact that bazel keeps workers running between builds

bugs could be hard to track down when they are the result of built up state

1️⃣ doesn’t seem likely, as this worker doesn't do much, but it might be worth experimenting with Xmx values

2️⃣ this worker doesn't carry any meaningful state beyond just the JVM’s own state

jastice · 2022-07-22T11:24:01Z

aspect/intellij_info_impl.bzl

@@ -704,6 +712,10 @@ def build_java_package_manifest(ctx, target, source_files, suffix):
        arguments = [args],
        mnemonic = "JavaPackageManifest",
        progress_message = "Parsing java package strings for " + str(target.label),
+        execution_requirements = {


@keithl-stripe is this something that makes sense from your perspective?

tpasternak · 2022-07-22T13:19:35Z

aspect/tools/src/com/google/idea/blaze/aspect/PackageParser.java

+        parsePackagesAndWriteManifest(parser, workRequestOptions);
+
+        builder.build().writeDelimitedTo(System.out);
+      } catch (Exception e) {


I'm wondering whether we should catch all exceptions this way here. In case an exception is thrown, the loop will still run. Maybe we should allow continuation of the loop only when "parsePackagesAndWriteManifest" throws a parsing-specific exception, and for other errors break the loop. The BazelJavaBuilder is doing so

https://github.com/bazelbuild/bazel/blob/a4251eab6988d6cf4f5e35681fbe2c1b0abe48ef/src/java_tools/buildjar/java/com/google/devtools/build/buildjar/BazelJavaBuilder.java#L45-L55

But I'm not sure, otherwise LGTM 👍

I wanted to reduce the risk that we crash hard for an issue that only affects the current action. I honestly don't know what is best here. If you'd like me to be more conservative that's fine - will update the PR.

Personally I would prefer to follow the same way as BazelJavaBuilder. For recoverable exceptions we can continue, but for unexpected ones System.exit would be more transparent. We could also print a message telling the user it is possible to run it without persistent workers.

Bencodes · 2022-07-22T19:52:19Z

@keithl-stripe There are a few other actions in this repo like CreateAar and JarFilter that would benefit greatly from having worker support. Can this worker implementation be made a bit more generic so that we can support those as well?

mai93 · 2022-07-29T20:32:04Z

@estebandlc I would like to get your feedback on this PR.

sgowroji · 2022-10-06T07:40:57Z

Hello @keithl-stripe, Could you please resolve the above code conflicts and help us in submitting it. Thanks!

keithl-stripe · 2022-10-11T15:24:46Z

Sorry, we are moving away from using this plugin here at Stripe, so we are investing our effort in our homegrown IntelliJ plugin. Someone else will have to pick up this PR and get it landed if people think it would be useful. (It sped up our sync builds substantially!)

jastice · 2022-10-13T18:20:23Z

Okay, then let's keep this open as a work item for maintainers or other contributors. Thank you for the effort so far!

tpasternak · 2023-05-10T08:16:50Z

If there are no other objections let's merge it now
cc @mai93

mai93 · 2023-05-10T09:42:23Z

I'm ok with that, can you fix the conflict then merge it? It is already approved.

Improve performance of Java package parser by making it Bazel worker

a845f7c

keithl-stripe requested review from alice-ks and vsiva as code owners June 14, 2022 20:23

Bencodes reviewed Jun 14, 2022

View reviewed changes

mai93 requested a review from jastice July 7, 2022 17:46

jastice requested review from mai93 and tpasternak and removed request for vsiva and alice-ks July 21, 2022 15:28

jastice approved these changes Jul 22, 2022

View reviewed changes

tpasternak reviewed Jul 22, 2022

View reviewed changes

mai93 approved these changes Jul 22, 2022

View reviewed changes

mai93 requested a review from estebandlc July 29, 2022 20:26

sgowroji added product: IntelliJ IntelliJ plugin topic: sync Issues related to the sync operation awaiting-review Awaiting review from Bazel team on PRs labels Sep 2, 2022

sgowroji added awaiting-user-response Awaiting response from author on PRs and removed awaiting-review Awaiting review from Bazel team on PRs labels Oct 6, 2022

mai93 self-assigned this Oct 6, 2022

jastice removed the awaiting-user-response Awaiting response from author on PRs label Oct 13, 2022

mai93 removed their assignment Oct 18, 2022

sgowroji assigned jastice Oct 19, 2022

mai93 added the awaiting-review Awaiting review from Bazel team on PRs label Dec 5, 2022

mai93 self-assigned this May 4, 2023

mai93 removed their assignment May 9, 2023

Merge branch 'master' into package-parser-worker

f9c7cbf

tpasternak force-pushed the package-parser-worker branch from 97762b3 to f9c7cbf Compare May 10, 2023 10:35

tpasternak approved these changes May 10, 2023

View reviewed changes

tpasternak merged commit e0efd6a into bazelbuild:master May 10, 2023
6 checks passed

mai93 mentioned this pull request May 10, 2023

Improve performance of Java package parser by making it a Bazel worker #3685

Closed

Bencodes mentioned this pull request May 10, 2023

Improve performance of JVM based actions by making them a Bazel worker compatible #4860

Closed

mai93 mentioned this pull request May 15, 2023

Update changelog for v2023.05.16 release #4875

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance of Java package parser by making it a Bazel worker #3686

Improve performance of Java package parser by making it a Bazel worker #3686

keithl-stripe commented Jun 14, 2022 •

edited

Bencodes Jun 14, 2022

jastice Jul 22, 2022

keithl-stripe Jul 25, 2022

Bencodes commented Jul 6, 2022

jastice left a comment •

edited

jastice Jul 22, 2022

tpasternak Jul 22, 2022

keithl-stripe Jul 25, 2022

tpasternak Jul 26, 2022

Bencodes commented Jul 22, 2022

mai93 commented Jul 29, 2022

sgowroji commented Oct 6, 2022

keithl-stripe commented Oct 11, 2022

jastice commented Oct 13, 2022

tpasternak commented May 10, 2023 •

edited

mai93 commented May 10, 2023

Improve performance of Java package parser by making it a Bazel worker #3686

Improve performance of Java package parser by making it a Bazel worker #3686

Conversation

keithl-stripe commented Jun 14, 2022 • edited

Checklist

Discussion thread for this change

Description of this change

Bencodes Jun 14, 2022

Choose a reason for hiding this comment

jastice Jul 22, 2022

Choose a reason for hiding this comment

keithl-stripe Jul 25, 2022

Choose a reason for hiding this comment

Bencodes commented Jul 6, 2022

jastice left a comment • edited

Choose a reason for hiding this comment

jastice Jul 22, 2022

Choose a reason for hiding this comment

tpasternak Jul 22, 2022

Choose a reason for hiding this comment

keithl-stripe Jul 25, 2022

Choose a reason for hiding this comment

tpasternak Jul 26, 2022

Choose a reason for hiding this comment

Bencodes commented Jul 22, 2022

mai93 commented Jul 29, 2022

sgowroji commented Oct 6, 2022

keithl-stripe commented Oct 11, 2022

jastice commented Oct 13, 2022

tpasternak commented May 10, 2023 • edited

mai93 commented May 10, 2023

keithl-stripe commented Jun 14, 2022 •

edited

jastice left a comment •

edited

tpasternak commented May 10, 2023 •

edited