Skip to content

Only iterate across active targets in WatchChangeAggregator #1422

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

Merged
merged 2 commits into from
Apr 3, 2020

Conversation

thebrianchen
Copy link

Porting from web.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 3, 2020

Coverage Report

Affected SDKs

SDKTypeBase (f0bda45)Head (45905fc)Diff
Metric Unit: percentage

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage. Report files are located at <product-build-dir>/reports/jacoco/.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Apr 3, 2020

Binary Size Report

Affected SDKs

SDKTypeBase (f0bda45)Head (45905fc)Diff
firebase-inappmessagingapk (aggressive)852137.00852136.00-1.00 (-0.00%)
protolite-well-known-typesapk (aggressive)122381.00122388.00+7.00 (+0.01%)
firebase-segmentationapk (aggressive)1017134.001017148.00+14.00 (+0.00%)
firebase-storageapk (aggressive)325634.00325628.00-6.00 (-0.00%)
firebase-commonapk (aggressive)82954.0082958.00+4.00 (+0.00%)
firebase-crashlytics-ndkapk (aggressive)1170702.001170685.00-17.00 (-0.00%)
firebase-databaseapk (aggressive)325613.00325596.00-17.00 (-0.01%)
firebase-crashlyticsapk (aggressive)583646.00583636.00-10.00 (-0.00%)
firebase-installationsapk (aggressive)84604.0084587.00-17.00 (-0.02%)
firebase-installations-interopapk (aggressive)61703.0061717.00+14.00 (+0.02%)
firebase-componentsapk (aggressive)10963.0010964.00+1.00 (+0.01%)
firebase-abtapk (aggressive)85721.0085720.00-1.00 (-0.00%)
firebase-configapk (aggressive)395836.00395821.00-15.00 (-0.00%)
firebase-datatransportapk (aggressive)116361.00116368.00+7.00 (+0.01%)
firebase-inappmessaging-displayapk (aggressive)1813858.001813848.00-10.00 (-0.00%)
firebase-functionsapk (aggressive)393476.00393473.00-3.00 (-0.00%)
firebase-database-collectionapk (aggressive)313624.00313609.00-15.00 (-0.00%)
firebase-firestoreapk (release)3139586.003139628.00+42.00 (+0.00%)
aar1067163.001067286.00+123.00 (+0.01%)
apk (aggressive)443195.00443188.00-7.00 (-0.00%)
baseapk (aggressive)10665.0010679.00+14.00 (+0.13%)
Metric Unit: byte

Test Logs

Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a nit

@@ -162,7 +163,13 @@ public void handleTargetChange(WatchTargetChange targetChange) {
if (!targetIds.isEmpty()) {
return targetIds;
} else {
return targetStates.keySet();
List<Integer> activeIds = new ArrayList<>();
for (int id : targetStates.keySet()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

targetStates is a HashMap with Integer keys. By iterating on this with an int variable you're unboxing and then re-boxing to add to activeIds. This should be for (Integer id : ...).

This whole file is sloppy about this but let's not make the problem any worse.

@wilhuff wilhuff assigned thebrianchen and unassigned wilhuff Apr 3, 2020
@thebrianchen thebrianchen merged commit 4491f0a into master Apr 3, 2020
@thebrianchen thebrianchen deleted the bc/active-target branch April 3, 2020 19:29
@firebase firebase locked and limited conversation to collaborators May 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants