Don't build unnecessary jars. #5515

nkoroste · 2023-10-20T21:06:48Z

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: #1785

Description of this change

reopened #1786 - Don't build the class jar if source jars are available, as we can use that in conjunction with the interface jar to index the code instead.

For our app it reduces ~30% of the Jars the IDE will index (from 14484 to 10072)

As per PR review request I put this feature behind optimize_building_jars experiment which is enabled by default. In case an issue is found with this feature this experiment can be disabled by disabled it in the IntelliJ experiments file as follows:

echo optimize_building_jars=0 >> ~/.intellij-experiments

agluszak · 2023-11-06T11:02:52Z

Hi!

For our app it reduces ~30% of the Jars the IDE will index (from 14484 to 10072)

Would you be willing to run some benchmarks on open source projects and provide the results here?

blorente

The implementation looks good to me, and I think it's unlikely to interfere with indexing. However, as @agluszak said, given that this changes a lot of the core resolution, it would be interesting to run the same benchmark in other projects (such as this repository, for instance) before merging, to make sure it's not a regression.

aspect/intellij_info_impl.bzl

nkoroste · 2023-11-07T16:48:41Z

Hi!

For our app it reduces ~30% of the Jars the IDE will index (from 14484 to 10072)

Would you be willing to run some benchmarks on open source projects and provide the results here?

and

The implementation looks good to me, and I think it's unlikely to interfere with indexing. However, as @agluszak said, given that this changes a lot of the core resolution, it would be interesting to run the same benchmark in other projects (such as this repository, for instance) before merging, to make sure it's not a regression.

I can provide a rough count of how many Jars would be ignored however I don't know how interesting that number is, instead what would be more interesting is to measure the impact this may have on indexing and code-completion time but unfortunately Intellij IDE itself nor this plugin provide an easy way to get those numbers

tpasternak · 2023-11-07T16:55:16Z

If the change is risky it would be good to at least provide some way to recover from it, ex. via a feature flag. Otherwise users will be blocked until we push a hotfix release

mai93 · 2023-11-07T19:06:13Z

Can we try to incrementally roll it out?, I feel like we have been adding a lot of feature flags and not all of them are documented so not a lot of users get to try the features they protect.

tpasternak · 2023-11-07T19:13:45Z

Can we try to incrementally roll it out?

Sure, how to do it?

I feel like we have been adding a lot of feature flags and not all of them are documented so not a lot of users get to try the features they protect.

Sorry, I know I was supposed to document the flags 😅 putting it on my priority list.

mai93 · 2023-11-07T19:26:04Z

We can use FeatureRolloutExperiment to define the experiment flag and set the roll out percentage in experiment.properties

mai93 · 2023-12-28T20:06:23Z

Follow up on how we can select between the old and new behaviour in the aspect, we can use --aspects_parameters to pass a boolean value to the aspect based on the experiment value (i.e. enabled or not) and select the one of the 2 approaches in the aspect based on this boolean. We need to add a boolean attribute in the aspect definition, can be called optimize_building_jars and pass its value from the command line as --aspects_parameters=optimize_building_jars=true.

aspect/testing/tests/src/com/google/idea/blaze/aspect/java/dependencies/DependenciesTest.java

nkoroste · 2024-07-16T05:07:16Z

PTAL @mai93 @tpasternak

…endencies/DependenciesTest.java

…tring' and use the 'values' restriction.

mai93 · 2024-07-18T01:00:29Z

aspect/testing/tests/src/com/google/idea/blaze/aspect/java/javabinary/JavaBinaryTest.java

@@ -58,10 +58,8 @@ public void testJavaBinary() throws Exception {
            testRelative(intellijInfoFileName("foo")));
    assertThat(getOutputGroupFiles(testFixture, "intellij-resolve-java"))
        .containsAtLeast(
-            testRelative("libfoolib.jar"),


why these jars are no longer expected, is optimization enabled for this test? same for DependenciesTest

Reverted these changes, they were traces to when optimization was enabled by default.

mai93 · 2024-07-19T01:10:04Z

it looks good to me, I can merge it if none else wants to review cc @tpasternak @agluszak

aschott-looker · 2024-08-19T23:08:19Z

This option isn't supported on the version of Bazel we use. How do you disable it for a project?

mai93 · 2024-08-19T23:16:50Z

what feature is not supported in the Bazel version you use?

@nkoroste mentioned that this experiment can be disabled by disabled it in the IntelliJ experiments file as follows:

echo optimize_building_jars=0 >> ~/.intellij-experiments

aschott-looker · 2024-08-20T00:40:15Z

Any Bazel version older than when this commit landed will fail:

ERROR: --aspects_parameters=optimize_building_jars=enabled :: Unrecognized option: --aspects_parameters=optimize_building_jars=enabled

tpasternak · 2024-08-20T11:33:38Z

@mai93 , @ujohnny do we still support Bazel 5?

tpasternak · 2024-08-20T11:35:27Z

Bazel 5 will be EOL in September. https://blog.bazel.build/2020/11/10/long-term-support-release.html

@aschott-looker are there any chances you could upgrade to bazel 6?

hisener · 2024-08-20T12:42:22Z

https://bazel.build/release says it's EOL in Jan 2025.

aschott-looker · 2024-08-20T16:18:41Z

@tpasternak We definitely want to upgrade to a more recent version of Bazel but it's not feasible in the short term. So I was hoping for a quick workaround.

Thanks for creating the issues above!

tpasternak · 2024-08-20T16:33:46Z

the hotfix is on its way #6653

nkoroste requested review from mai93, jastice, tpasternak and agluszak as code owners October 20, 2023 21:06

github-actions bot added the awaiting-review Awaiting review from Bazel team on PRs label Oct 20, 2023

sgowroji added the product: IntelliJ IntelliJ plugin label Oct 25, 2023

sgowroji assigned mai93 Oct 25, 2023

mai93 assigned tpasternak and blorente and unassigned mai93 Oct 25, 2023

blorente approved these changes Nov 6, 2023

View reviewed changes

aspect/intellij_info_impl.bzl Outdated Show resolved Hide resolved

nkoroste force-pushed the nk-reduceSyncJars-external branch from 1ec913d to 1d786c2 Compare June 29, 2024 00:25

nkoroste commented Jul 16, 2024

View reviewed changes

aspect/testing/tests/src/com/google/idea/blaze/aspect/java/dependencies/DependenciesTest.java Outdated Show resolved Hide resolved

uri-canva and others added 6 commits July 16, 2024 23:27

Don't build unnecessary jars.

1bf4f66

Remove unnecessary assignment

2bec267

Put feature behind a flag/expirement

6fd235f

fix

9c9d24c

Update aspect/testing/tests/src/com/google/idea/blaze/aspect/java/dep…

909990a

…endencies/DependenciesTest.java

Aspect parameter attribute 'optimize_building_jars' must have type 's…

25b5c65

…tring' and use the 'values' restriction.

nkoroste force-pushed the nk-reduceSyncJars-external branch from bf1696c to 25b5c65 Compare July 17, 2024 06:32

mai93 reviewed Jul 18, 2024

View reviewed changes

revert test modifcation

71ea02c

mai93 merged commit 2036193 into bazelbuild:master Jul 23, 2024
6 checks passed

github-actions bot removed the awaiting-review Awaiting review from Bazel team on PRs label Jul 23, 2024

mai93 mentioned this pull request Jul 29, 2024

Update changelog for v2024.07.30 release #6602

Merged

nkoroste mentioned this pull request Aug 6, 2024

"Don't build unnecessary jars" causes built-with-aspect failure #6614

Closed

This was referenced Aug 20, 2024

Sync Project with BUILD files fails with Error: Unrecognized option: --aspects_parameters=optimize_building_jars=enabled #6650

Closed

Introduce a RepositoryOverridesExperimentLoader #6651

Open

mai93 mentioned this pull request Aug 29, 2024

Enable not building unnecessary jars by default #6685

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't build unnecessary jars. #5515

Don't build unnecessary jars. #5515

nkoroste commented Oct 20, 2023 •

edited

Loading

agluszak commented Nov 6, 2023 •

edited

Loading

blorente left a comment

nkoroste commented Nov 7, 2023

tpasternak commented Nov 7, 2023

mai93 commented Nov 7, 2023

tpasternak commented Nov 7, 2023

mai93 commented Nov 7, 2023

mai93 commented Dec 28, 2023

nkoroste commented Jul 16, 2024

mai93 Jul 18, 2024

nkoroste Jul 18, 2024

mai93 commented Jul 19, 2024

aschott-looker commented Aug 19, 2024

mai93 commented Aug 19, 2024

aschott-looker commented Aug 20, 2024

tpasternak commented Aug 20, 2024

tpasternak commented Aug 20, 2024

hisener commented Aug 20, 2024

aschott-looker commented Aug 20, 2024

tpasternak commented Aug 20, 2024

Don't build unnecessary jars. #5515

Don't build unnecessary jars. #5515

Conversation

nkoroste commented Oct 20, 2023 • edited Loading

Checklist

Discussion thread for this change

Description of this change

agluszak commented Nov 6, 2023 • edited Loading

blorente left a comment

Choose a reason for hiding this comment

nkoroste commented Nov 7, 2023

tpasternak commented Nov 7, 2023

mai93 commented Nov 7, 2023

tpasternak commented Nov 7, 2023

mai93 commented Nov 7, 2023

mai93 commented Dec 28, 2023

nkoroste commented Jul 16, 2024

mai93 Jul 18, 2024

Choose a reason for hiding this comment

nkoroste Jul 18, 2024

Choose a reason for hiding this comment

mai93 commented Jul 19, 2024

aschott-looker commented Aug 19, 2024

mai93 commented Aug 19, 2024

aschott-looker commented Aug 20, 2024

tpasternak commented Aug 20, 2024

tpasternak commented Aug 20, 2024

hisener commented Aug 20, 2024

aschott-looker commented Aug 20, 2024

tpasternak commented Aug 20, 2024

nkoroste commented Oct 20, 2023 •

edited

Loading

agluszak commented Nov 6, 2023 •

edited

Loading