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

Revert "Add ServerValue.increment() methods." #1104

Closed

Conversation

schmidt-sebastian
Copy link
Contributor

Reverts #991

Reverting this change as it likely has performance implications (as shown in firebase/firebase-js-sdk#2487)

@schmidt-sebastian
Copy link
Contributor Author

cc @inlined

@codecov
Copy link

codecov bot commented Jan 8, 2020

Codecov Report

Merging #1104 into master will increase coverage by 1.22%.
The diff coverage is 35.48%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1104      +/-   ##
============================================
+ Coverage     57.12%   58.35%   +1.22%     
- Complexity     6218     6318     +100     
============================================
  Files           640      640              
  Lines         31446    31397      -49     
  Branches       4335     4331       -4     
============================================
+ Hits          17965    18322     +357     
+ Misses        12056    11621     -435     
- Partials       1425     1454      +29
Impacted Files Coverage Δ Complexity Δ
...java/com/google/firebase/database/ServerValue.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...va/com/google/firebase/database/core/SyncTree.java 57.72% <100%> (-4.53%) 35 <0> (-5)
...n/java/com/google/firebase/database/core/Repo.java 10.87% <30%> (+0.4%) 13 <0> (ø) ⬇️
...om/google/firebase/database/core/ServerValues.java 59.18% <33.33%> (+20.22%) 8 <4> (+1) ⬆️
...abase/core/view/filter/ChildChangeAccumulator.java 61.29% <0%> (-12.91%) 11% <0%> (-1%)
...a/com/google/firebase/database/core/WriteTree.java 66.29% <0%> (-9.95%) 47% <0%> (-6%)
...google/firebase/database/snapshot/BooleanNode.java 92.3% <0%> (-7.7%) 13% <0%> (-1%)
...om/google/firebase/storage/StorageTaskManager.java 94.59% <0%> (-2.71%) 14% <0%> (-1%)
.../com/google/firebase/storage/StorageException.java 52.72% <0%> (ø) 23% <0%> (+1%) ⬆️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa9a8c7...8a078de. Read the comment docs.

Copy link
Contributor

@mikelehen mikelehen left a comment

Choose a reason for hiding this comment

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

Revert looks fine, though I think the update() codepath ends up hitting

entry.getValue(), existing.getChild(entry.getKey()), serverValues));

which doesn't have the same bug that web had. So the perf regression likely isn't present.

@schmidt-sebastian
Copy link
Contributor Author

Closing this for now to reduce churn, especially since we haven't seen evidence of performance regressions on Android. Let's try to fix JS first and if there are any changes, we can backport them.

@schmidt-sebastian schmidt-sebastian deleted the revert-991-inlined.servervalue-increment branch February 7, 2020 00:22
@firebase firebase locked and limited conversation to collaborators Feb 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Override cla size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants