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

calculateSha in gradle.dart has been reported to take > a minute #29562

Closed
jonahwilliams opened this issue Mar 18, 2019 · 19 comments
Closed

calculateSha in gradle.dart has been reported to take > a minute #29562

jonahwilliams opened this issue Mar 18, 2019 · 19 comments
Assignees
Labels
c: performance Relates to speed or footprint issues (see "perf:" labels) t: gradle "flutter build" and "flutter run" on Android tool Affects the "flutter" command-line tool. See also t: labels.

Comments

@jonahwilliams
Copy link
Member

It seems unfortunate if we spend as much time calculating a sha as we do running gradle. Investigate what this file is used for and whether we can make it faster

@jonahwilliams jonahwilliams added tool Affects the "flutter" command-line tool. See also t: labels. c: performance Relates to speed or footprint issues (see "perf:" labels) labels Mar 18, 2019
@jonahwilliams jonahwilliams self-assigned this Mar 18, 2019
@jason-simmons
Copy link
Member

The SHA hash is used to check whether the APK that is already installed on the device is identical to the one in the host's build output directory.

After installing an APK, flutter-tools writes a file in a temp directory on the device containing a hash of the APK. If you run flutter run again, flutter-tools will compare the hash on the device to a hash of the local APK and skip the adb install if the values match.

It looks like there have been previous reports of slow performance for the Dart crypto/hash algorithms (see dart-lang/crypto#5). I saw that a CRC package is available (https://pub.dartlang.org/packages/crclib) - maybe this would be faster?

Otherwise we could always install the APK during flutter run. I don't know how common it is to do repeated runs of flutter run with no changes to the app code.

@feinstein
Copy link
Contributor

feinstein commented Mar 18, 2019

What about instead of calculating the hash of an entire APK, just generate a build number at each compilation and compare the build numbers?

Those build numbers would have to stay the same in case there was no change to any files (maybe marking the code as dirty in case there's any modifications from the previous build and increasing the build number in case the code is dirty?).

Is this possible, or easier said than done?

It could be a drastical performance increase.

@jonahwilliams
Copy link
Member Author

I think if this can be solved by adjusting the hashing algorithm, then I would prefer that over a more intrusive fix

@feinstein
Copy link
Contributor

Yeah, it makes sense, it's a drop-in solution and CRC should be a lot faster.

It would be nice to document why CRC is there, so someone in the future don't try to "improve" the hashing algorithm to SHA512 :)

@jonahwilliams
Copy link
Member Author

Well, I investigated a few different solutions and found that they all started to take more than a minute once we got to larger app sizes. I think the best solution is to skip the check and always install, since that takes dramatically less time then checking.

While the build number solution might be more efficient, I'd be concerned about correctness

@feinstein
Copy link
Contributor

What about giving the users the ability to bypass instalation if both APKs have the same size? This could be a checkbox under experimental options, users would be aware of it, and if they want to force install they just have to unselect it.

I am not aware of the probability of having 2 APKs with the same size and different code, but I would bet it's kinda small, specially with incremental development.

@jonahwilliams
Copy link
Member Author

My concern with that is the failure mode is uncommon but hard to impossible diagnose for a user.

@feinstein
Copy link
Contributor

feinstein commented Apr 24, 2019

There could be a message on the console saying:

Not installing the APK since APK on the device and the compiled APK have the same size. If you want to force install (insert here link to docs or explanation on how to proceed)

I guess users will get used with this behavior, since they will see this a lot.

@csells csells changed the title calculateSha in gradle.dart has been reported to take more than a minute calculateSha in gradle.dart has been reported to take > a minute Jan 9, 2020
@jmagman jmagman added this to Awaiting triage in Tools - Gradle review via automation Jan 11, 2020
@jmagman jmagman added the t: gradle "flutter build" and "flutter run" on Android label Jan 11, 2020
@jonahwilliams
Copy link
Member Author

I think we're going to try to solve this a different way, by making fast-start a better option. Locally, we've been unable to produce exceptionally long calculateSha times, but I can imagine on certain hardware combinations gradle can be quite slow

Tools - Gradle review automation moved this from Awaiting triage to Engineer reviewed Feb 5, 2020
@feinstein
Copy link
Contributor

I am no problem with this being closed, I just want to ask if you guys at Google test with your own machines, or if you also have other machines at your disposal that represent the spectrum of machines used by the developers worldwide?

@jonahwilliams
Copy link
Member Author

We measure the performance of tool users via google analytics, if you've opted into them. This allows us to see global averages / 90% and 99%

@feinstein
Copy link
Contributor

Do you see just averages or percentile as well?

@jonahwilliams
Copy link
Member Author

percentile. At any rate, I've spent a lot of time this year trying to make the build faster for all users. But this particular tree (calculateSha) has not born any fruit.

@feinstein
Copy link
Contributor

I understand that, my only concern is that I usually see Google devs with an expensive Macbook and many of us out here have a totally different experience with average windows computers, so I was just wondering if the Silicon Valley experience wasn't hiding the performance issues we see out in the wild.... But I guess my 2016 Dell Inspiron 13 7000 is just an edge case then, thanks Jonah.

@jonahwilliams
Copy link
Member Author

You're definitely not an edge case, but we haven't been able to consistently improve the performance of this method. In general, I'm not willing to trade performance for correctness here - since the alternative is very difficult to diagnose issues.

I am still working on fast-start and related performance improvements

@feinstein
Copy link
Contributor

I agree fast-start should be the way to go and should solve all of this.

@jonahwilliams
Copy link
Member Author

jonahwilliams commented Feb 5, 2020

FWIW, I do have an expensive macbook - I also have a windows desktop that I built in 2009 with a HDD. I'm very much aware of the performance gradient

@feinstein
Copy link
Contributor

It's always important to stick to our roots and remember where we came from :P

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: performance Relates to speed or footprint issues (see "perf:" labels) t: gradle "flutter build" and "flutter run" on Android tool Affects the "flutter" command-line tool. See also t: labels.
Projects
Tools - Gradle review
  
Engineer reviewed
Development

No branches or pull requests

4 participants