Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Significant target-resolution runtime regression #670

Closed
HannesWell opened this issue Feb 21, 2022 · 14 comments
Closed

Significant target-resolution runtime regression #670

HannesWell opened this issue Feb 21, 2022 · 14 comments
Labels
🚨 regression Regression compared to previous release - blocks upcoming release and other merges
Milestone

Comments

@HannesWell
Copy link
Member

As reported in #651 (comment) the initial resolution of the target platform is magnitudes slower with the current Tycho 2.7.0-SNAPSHOT compared to 2.6.0, in my case ~30min compared to ~30sec before.

I have the impression that each dependency of each plug-in being built is fully fetched from a mirror again, even if it was already fetched for another plug-in which has the same dependency in the same Maven build. Therefore the runtime becomes linear to the number of references to dependencies and not only linear to the cardinality of the set of dependencies (which would be relatable).
Furthermore the dependencies should not be re-downloaded on each build which happens often (or maybe always, I cannot say that clearly from the print-out).

Please find attached a relativly small reproducer that contains three identical plug-ins that have the same dependencies:
tycho-2-7.zip

@HannesWell
Copy link
Member Author

@laeubi I cannot assign labels here, likely because I'm not a committer for Tycho.

@laeubi laeubi added this to the 2.7 milestone Feb 21, 2022
@laeubi laeubi added the 🚨 regression Regression compared to previous release - blocks upcoming release and other merges label Feb 21, 2022
@HannesWell
Copy link
Member Author

Furthermore .jar.pack.gz dependencies are downloaded, although I did not (intent) to enable it.

@laeubi
Copy link
Member

laeubi commented Feb 21, 2022

The jar.pack.gz is because RemoteArtifactTransferPolicy prefers pack.gz

@akurtakov @mickaelistria given that we remove pack200 in tycho 3.0 I'd like to change this so we simply never use pack200 in tycho 2.7. anymore even if it would be supported by the running JVM... any concerns?

@akurtakov
Copy link
Member

Please ignore pack.gz files. They are already ignored if running on Java13+ IIRC

laeubi added a commit to laeubi/tycho that referenced this issue Feb 21, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@laeubi
Copy link
Member

laeubi commented Feb 21, 2022

I was able to reduce this to one download per build, but this is a really nasty problem... now that we are using the remote properties the following happens:

  1. We request an "p2 scoped" dependency while target resolution, this misses the properties of course
  2. We then schedule a download of the artifact and store that at the (now determined by maven coordinates) different place
  3. on the next build the cycle starts over again...

laeubi added a commit to laeubi/tycho that referenced this issue Feb 21, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
laeubi added a commit to laeubi/tycho that referenced this issue Feb 21, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
laeubi added a commit to laeubi/tycho that referenced this issue Feb 21, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
laeubi added a commit to laeubi/tycho that referenced this issue Feb 21, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
laeubi added a commit to laeubi/tycho that referenced this issue Feb 22, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@laeubi
Copy link
Member

laeubi commented Feb 22, 2022

Please ignore pack.gz files. They are already ignored if running on Java13+ IIRC

Currently there are too many test that rely on pack200 :-\
But I think I found a solution for this lets see if all integration tests are still running....

@akurtakov
Copy link
Member

All tests are fixed in my pack200 patch against master. Maybe it's best to leave its removal for 3.0

@laeubi
Copy link
Member

laeubi commented Feb 22, 2022

Yes removal for 3.0 and as this issue must also be done in 2.7 I decided to keep everything as-is and still support pack200 at the level of 2.7, hopefully after the release we then can remove this stuff completely....

laeubi added a commit to laeubi/tycho that referenced this issue Feb 22, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
laeubi added a commit to laeubi/tycho that referenced this issue Feb 22, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
laeubi added a commit to laeubi/tycho that referenced this issue Feb 22, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
laeubi added a commit to laeubi/tycho that referenced this issue Feb 22, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@HannesWell
Copy link
Member Author

HannesWell commented Feb 22, 2022

I was able to reduce this to one download per build, but this is a really nasty problem... now that we are using the remote properties the following happens:

1. We request an "p2 scoped" dependency while target resolution, this misses the properties of course

2. We then schedule a download of the artifact and store that at the (now determined by maven coordinates) different place

3. on the next build the cycle starts over again...

Sounds nasty. But one download per build (the state of PR #673 this noon) already reduced the resolution time to ~4min in my case compared to more than 30min before. But it is still much longer than the 30sec of Tycho 2.6.0.

@laeubi
Copy link
Member

laeubi commented Feb 22, 2022

I hope I have catches all cases now wit the PR and everything goes fine. If yes, only one download should happen once to get in sync and after that everything should be cached again...

@HannesWell
Copy link
Member Author

I hope I have catches all cases now wit the PR and everything goes fine. If yes, only one download should happen once to get in sync and after that everything should be cached again...

Great job @laeubi!
I just tested the latest state of your PR #673 back-ported to the 2.7 branch with my case and the resolution is now as fast as with Tycho 2.6.0 (maybe even slightly faster) and I did not found any Fetching print-out after the first build.
Thanks again.

@akurtakov
Copy link
Member

akurtakov commented Feb 23, 2022

My local build of https://github.com/eclipse-platform/eclipse.platform.releng (mvn clean verify -Pbuild-individual-bundles) keeps redownloading stuff . SWT fragments are the first visible part noticed on every build.

@laeubi
Copy link
Member

laeubi commented Feb 23, 2022

My local build of https://github.com/eclipse-platform/eclipse.platform.releng (mvn clean verify -Pbuild-individual-bundles) keeps redownloading stuff . SWT fragments are the first visible part noticed on every build.

Ho do I change the tycho version to be used here? Can you try to reproduce the issue with #680

laeubi added a commit to laeubi/tycho that referenced this issue Feb 23, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@akurtakov
Copy link
Member

My local build of https://github.com/eclipse-platform/eclipse.platform.releng (mvn clean verify -Pbuild-individual-bundles) keeps redownloading stuff . SWT fragments are the first visible part noticed on every build.

Ho do I change the tycho version to be used here? Can you try to reproduce the issue with #680

Just pass -Dtycho.version=3.0.0-SNAPSHOT

laeubi added a commit to laeubi/tycho that referenced this issue Feb 23, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
laeubi added a commit to laeubi/tycho that referenced this issue Feb 23, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
laeubi added a commit to laeubi/tycho that referenced this issue Feb 25, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@laeubi laeubi closed this as completed in 3b059c6 Feb 25, 2022
laeubi added a commit that referenced this issue Feb 25, 2022
Signed-off-by: Christoph Läubrich <laeubi@laeubi-soft.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🚨 regression Regression compared to previous release - blocks upcoming release and other merges
Projects
None yet
Development

No branches or pull requests

3 participants