Implement ObjectArena and ArenaRef to solve JNI Global Reference Issue by cherylEnkidu · Pull Request #1176 · firebase/firebase-cpp-sdk

cherylEnkidu · 2022-12-25T06:39:15Z

Description

Implemented ArenaRef which uses as a replacement of Global Reference in order to solve problems caused by JNI Global Reference limitation.

Testing

Integration tests

Type of Change

Place an x the applicable box:

Bug fix. (#569)(#1193)
New feature. A non-breaking change which adds functionality.
Other, such as a build process or documentation change.

Notes

Bug fixes and feature changes require an update to the Release Notes section of release_build_files/readme.md.
Read the contribution guidelines CONTRIBUTING.md.
Changes to the public API require an internal API review. If you'd like to help us make Firebase APIs better, please propose your change in a feature request so that we can discuss it together.

github-actions · 2022-12-25T06:40:02Z

Integration test with FLAKINESS (succeeded after retry)

Requested by @cherylEnkidu on commit 2ce5b8d
Last updated: Sun Dec 25 09:59 PST 2022
View integration test log & download artifacts

Failures	Configs
firestore	[TEST] [FLAKINESS] [Android] [1/3 os: windows] [1/2 android_device: android_target] (1 failed tests) CRASH/TIMEOUT

Add flaky tests to go/fpl-cpp-flake-tracker

github-actions · 2022-12-29T22:09:07Z

✅ Integration test succeeded!

Requested by @cherylEnkidu on commit 7889019
Last updated: Sun Feb 12 19:20 PST 2023
View integration test log & download artifacts

…cal ref

firestore/integration_test_internal/src/android/arena_ref_android_test.cc

firestore/src/android/document_reference_android.cc

firestore/src/android/field_value_android.cc

firestore/src/android/field_value_android.h

firestore/src/jni/arena_ref.cc

firestore/src/jni/arena_ref.h

firestore/integration_test_internal/src/android/arena_ref_android_test.cc

dconeybe · 2023-01-16T15:16:58Z

I just realized something: the ArenaRef has a concurrency bug. If ArenaRefs are created/copied concurrently from multiple threads then there is a race condition that will cause bugs. This is because the multiple threads can be accessing the Java HashMap at the same time without any synchronization. This leads to undefined behavior, and could be causing that weird bug you're seeing in CI that you can't reproduce locally. One fix would be to protect gArenaRefHashMap with an std::mutex.

…ad ignoring exception and only printing description

firestore/src/jni/arena_ref.cc

firestore/integration_test_internal/src/android/arena_ref_android_test.cc

firestore/src/jni/arena_ref.cc

awais-100 · 2023-02-06T11:14:02Z

Any update, please? Games crash rates are too high due to this issue, and Play Console's bad behavior threshold is crossed.

dconeybe · 2023-02-06T15:44:02Z

Any update, please? Games crash rates are too high due to this issue, and Play Console's bad behavior threshold is crossed.

Please see firebase/firebase-unity-sdk#569. tl;dr The fix is not yet ready but we'll let you know when it is.

firestore/src/jni/arena_ref.cc

firestore/src/jni/arena_ref.h

dconeybe · 2023-02-13T15:39:13Z

firestore/src/jni/arena_ref.cc

+
+ArenaRef& ArenaRef::operator=(ArenaRef&& other) {
+  if (this != &other) {
+    std::swap(key_, other.key_);


Please do the using std::swap thing here in the move assignment operator, just like you did for the move constructor (#1176 (comment))

dconeybe · 2023-02-13T15:45:15Z

firestore/integration_test_internal/src/jni/env_test.cc

  EXPECT_EQ("Foo", result);
 }

+TEST_F(EnvTest, IsInstanceOfChecksValidArenaRef) {


Augment the IsInstanceOfChecksValidArenaRef test to also call IsInstanceOf with a different class (e.g. java.lang.Integer) such that it returns false.

dconeybe · 2023-02-13T15:48:49Z

firestore/integration_test_internal/src/field_value_test.cc

  // clang-format on
 }

+TEST_F(FieldValueTest, TestArenaRefMinimunLimit) {


Please add a comment to TestArenaRefMinimunLimit to mention the global refs issue that it's checking and include a link to the issue that it's verifying doesn't regress (i.e. firebase/firebase-unity-sdk#569)

dconeybe · 2023-02-13T16:02:49Z

firestore/src/jni/arena_ref.cc

+HashMap* gArenaRefHashMap = nullptr;
+jclass gLongClass = nullptr;
+jmethodID gLongConstructor = nullptr;
+std::mutex mutex_;


nit: Move std::mutex mutex_ to be defined above the variable that it is protecting (i.e. above the definition of gArenaRefHashMap). Although it's not super relevant in this case, you always want to make sure that the mutex outlives the variable(s) that it is protecting.

dconeybe · 2023-02-13T16:03:23Z

firestore/src/jni/arena_ref.cc

+HashMap* gArenaRefHashMap = nullptr;
+jclass gLongClass = nullptr;
+jmethodID gLongConstructor = nullptr;
+std::mutex mutex_;


Rename mutex_ to match the naming convention of global variables. Also name it to indicate which variable it is protecting. e.g. gArenaRefHashMapMutex.

dconeybe · 2023-02-13T16:24:28Z

firestore/integration_test_internal/src/jni/arena_ref_test.cc

+    jmethodID ctor =
+        env().GetMethodId(clazz, "<init>", "(Ljava/lang/String;)V");
+
+    Local<String> message = env().NewStringUtf("Testing throw");


I think you can just get rid of these throwException() and clearExceptionOccurred() helper methods.

throwException() can be replaced by

env.Throw(CreateException(env, "Test Exception"));

and clearExceptionOccurred() can be replaced by

env.ExceptionClear();

Admittedly, this leads to a tiny bit of code duplciation between test cases but that's acceptable because, IMO, it improves readabililty of the test cases.

See go/tott/598 about "DAMP" tests rather than the "DRY" principle that is common in non-testing code.

dconeybe · 2023-02-13T16:28:20Z

firestore/integration_test_internal/src/jni/arena_ref_test.cc

+TEST_F(ArenaRefTestAndroid, MoveConstructsFromValid) {
+  Local<String> string = env().NewStringUtf("hello world");
+
+  ArenaRef arena_ref2(env(), string);


nit: Rename arena_ref2 and arena_ref3 back to arena_ref1 and arena_ref2, respectively.

dconeybe · 2023-02-13T16:30:31Z

firestore/integration_test_internal/src/jni/arena_ref_test.cc

+
+TEST_F(ArenaRefTestAndroid, ThrowBeforeConstruct) {
+  Local<String> string = env().NewStringUtf("hello world");
+  EXPECT_EQ(string.ToString(env()).size(), 11U);


Remove these checks for the length of the string returned from NewStringUtf(). You can just assume that this is true. Applies here and elsewhere in this file.

dconeybe · 2023-02-13T16:31:33Z

firestore/integration_test_internal/src/jni/arena_ref_test.cc

+  EXPECT_TRUE(env().IsSameObject(arena_ref1.get(env()), string1));
+}
+
+TEST_F(ArenaRefTestAndroid, ThrowBeforeConstruct) {


nit: Can you rename the "ThrowBeforeXXX" test names to "XXXWithPendingException" or something like that? I think using the term "with pending exception" more clearly conveys what scenario the test is testing.

dconeybe · 2023-02-13T16:32:41Z

firestore/integration_test_internal/src/jni/arena_ref_test.cc

+  clearExceptionOccurred();
+  EXPECT_TRUE(env().IsSameObject(arena_ref2.get(env()), string));
+}
+


Add a test for calling get() with a pending exception.

Add a skeleton for ArenaRef

3acc2af

cherylEnkidu self-assigned this Dec 25, 2022

cherylEnkidu added skip-release-notes Skip release notes check tests-requested: quick Trigger a quick set of integration tests. labels Dec 25, 2022

github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Dec 25, 2022

Create big five of ArenaRef.

8975580

cherylEnkidu force-pushed the cheryllin/AndroidJNI2 branch from 69db5b3 to 8975580 Compare December 25, 2022 06:43

cherylEnkidu added tests-requested: quick Trigger a quick set of integration tests. and removed tests: in-progress This PR's integration tests are in progress. labels Dec 25, 2022

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests-requested: quick Trigger a quick set of integration tests. labels Dec 25, 2022

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Dec 25, 2022

github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Dec 25, 2022

Use as example to show the efficiency drops down

d30becc

cherylEnkidu added tests-requested: quick Trigger a quick set of integration tests. and removed tests: succeeded This PR's integration tests succeeded. tests: failed This PR's integration tests failed. labels Dec 29, 2022

github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Dec 29, 2022

github-actions bot added the tests: failed This PR's integration tests failed. label Dec 29, 2022

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Dec 29, 2022

dconeybe added 3 commits January 3, 2023 12:33

fix crash in DocumentReferenceInternal::Set() due to using a stale lo…

8ef27ff

…cal ref

Remove invalid Wrapper::ToJava() overload for FieldValue

ebc739b

Merge branch 'main' into cheryllin/AndroidJNI2

ddf359c

dconeybe added tests-requested: quick Trigger a quick set of integration tests. and removed tests: failed This PR's integration tests failed. labels Jan 3, 2023

github-actions bot added tests: succeeded This PR's integration tests succeeded. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jan 12, 2023

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jan 12, 2023

dconeybe suggested changes Jan 13, 2023

View reviewed changes

Address Feedback 2

6c3fc50

cherylEnkidu added tests-requested: quick Trigger a quick set of integration tests. and removed tests: succeeded This PR's integration tests succeeded. labels Jan 18, 2023

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jan 18, 2023

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jan 18, 2023

Fix "Address Feedback 2"

54c0826

cherylEnkidu force-pushed the cheryllin/AndroidJNI2 branch from 4c9e2a6 to 54c0826 Compare January 19, 2023 08:54

cherylEnkidu added 2 commits January 20, 2023 16:27

Avoid throw exception in cpp destructor in the previous commit, inste…

7e14163

…ad ignoring exception and only printing description

Add mutex for HashMap inside ArenaRef

84cbd19

This was referenced Jan 30, 2023

[Bug] NewGlobalRef(void*) and other errors on Android firebase/firebase-unity-sdk#614

Closed

[Bug] Firestore Android using too many global refs firebase/firebase-unity-sdk#569

Closed

dconeybe suggested changes Jan 30, 2023

View reviewed changes

dconeybe suggested changes Jan 31, 2023

View reviewed changes

firestore/src/jni/arena_ref.cc Outdated Show resolved Hide resolved

firestore/src/jni/arena_ref.cc Outdated Show resolved Hide resolved

firestore/src/jni/arena_ref.cc Show resolved Hide resolved

firestore/src/jni/arena_ref.cc Outdated Show resolved Hide resolved

dconeybe reviewed Feb 8, 2023

View reviewed changes

firestore/src/jni/arena_ref.cc Outdated Show resolved Hide resolved

Address feedback

b599bfa

dconeybe reviewed Feb 10, 2023

View reviewed changes

firestore/src/jni/arena_ref.h Outdated Show resolved Hide resolved

cherylEnkidu added 2 commits February 12, 2023 17:18

Address feedback again

e7879c5

Add tests to prove the fix

84400cf

dconeybe suggested changes Feb 13, 2023

View reviewed changes

dconeybe mentioned this pull request May 16, 2023

Firestore: Fix global refs exhaustion issue in Android #1325

Closed

dconeybe mentioned this pull request Jun 27, 2023

Firestore: Fix global refs exhaustion issue in Android #1364

Merged

Conversation

cherylEnkidu commented Dec 25, 2022

Description

Testing

Type of Change

Notes

Uh oh!

github-actions bot commented Dec 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Integration test with FLAKINESS (succeeded after retry)

Uh oh!

github-actions bot commented Dec 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Integration test succeeded!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dconeybe commented Jan 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

awais-100 commented Feb 6, 2023

Uh oh!

dconeybe commented Feb 6, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Dec 25, 2022 •

edited

Loading

github-actions bot commented Dec 29, 2022 •

edited

Loading