Memory LRU GC #6943

wu-hui · 2023-01-11T20:11:34Z

Googlers see: go/firestore-memory-lru

changeset-bot · 2023-01-11T20:11:38Z

🦋 Changeset detected

Latest commit: 85483bf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
firebase	Minor
@firebase/firestore	Minor
@firebase/firestore-compat	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

google-oss-bot · 2023-01-11T20:40:12Z

Size Report ¹

Affected Products

`@firebase/firestore`

Type	Base (`1d6771e`)	Merge (`7a96fce`)	Diff
browser	278 kB	281 kB	+3.04 kB (+1.1%)
esm5	346 kB	350 kB	+3.83 kB (+1.1%)
main	554 kB	560 kB	+5.66 kB (+1.0%)
module	278 kB	281 kB	+3.04 kB (+1.1%)
react-native	279 kB	282 kB	+3.04 kB (+1.1%)

`bundle`

Type	Base (`1d6771e`)	Merge (`7a96fce`)	Diff
firestore (Persistence)	284 kB	284 kB	+106 B (+0.0%)
firestore (Query Cursors)	222 kB	222 kB	+20 B (+0.0%)
firestore (Query)	220 kB	220 kB	+20 B (+0.0%)
firestore (Read data once)	207 kB	207 kB	+20 B (+0.0%)
firestore (Realtime updates)	209 kB	209 kB	+20 B (+0.0%)
firestore (Transaction)	190 kB	190 kB	+20 B (+0.0%)
firestore (Write data)	190 kB	190 kB	+101 B (+0.1%)

firebase
Type Base (1d6771e) Merge (7a96fce) Diff
firebase-compat.js 757 kB 757 kB +24 B (+0.0%)
firebase-firestore-compat.js 324 kB 324 kB +24 B (+0.0%)
firebase-firestore.js 327 kB 330 kB +3.04 kB (+0.9%)

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/B53YMZxsjX.html

google-oss-bot · 2023-01-11T20:54:06Z

Size Analysis Report ¹

This report is too large (729,651 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/fFTP0ub9qj.html

egilmorez

A few tweaks to the public doc comments, thanks!

packages/firestore/src/api/database.ts

MarkDuckworth

LGTM with comments

MarkDuckworth · 2023-04-14T12:15:45Z

packages/firestore/src/api/cache_config.ts

+ *
+ * This collector tries to ensure lowest memory footprints from the SDK,
+ * at the risk of querying backend repeated for a document it could have
+ * cached locally.


I think this statement is misleading as it gives indication that having documents in the cache will retrieve documents stored in the cached and go to the backend when not in cache.

Consider instead "This collector tries to ensure lowest memory footprints from the SDK, at the risk of documents not being cached for offline queries or for direct queries to the cache."

MarkDuckworth · 2023-04-14T12:40:24Z

packages/firestore/src/core/component_provider.ts

+
+  createPersistence(cfg: ComponentConfiguration): Persistence {
+    const lruParams =
+      this.cacheSizeBytes !== undefined


wondering if we need a range check for this.cacheSizeBytes > 0

This actually won't work because we use -1 to disable GC.

I really do not like this approach, we should do something similar to the new cache config API, using a dedicated enum branch to disable GC. Maybe something for the fixit week.

MarkDuckworth · 2023-04-14T12:41:57Z

packages/firestore/src/core/firestore_client.ts

+     * (incoming GRPC messages), we should always schedule onto this queue. //
+     * This ensures all of our work is properly serialized (e.g. we don't //
+     * start processing a new operation while the previous one is waiting for //
+     * an async I/O to complete). //


Not sure if ending // were intentional

Ah, I hate it when my editor does this and i don't know how to stop it.

MarkDuckworth · 2023-04-14T12:45:54Z

packages/firestore/src/local/memory_persistence.ts

+    persistence: MemoryPersistence,
+    lruParams: LruParams | null
+  ): MemoryLruDelegate {
+    return new MemoryLruDelegate(persistence, lruParams!!);


what is the double exclamation at the end of lruParams!! do? I'm not familiar with the double.

It tells complier that you are sure this is a LruParams, not a null. I removed | null in this case, it was not neccessary.

MarkDuckworth · 2023-04-14T12:59:37Z

packages/firestore/test/unit/specs/spec_builder.ts

-  withGCEnabled(gcEnabled: boolean): this {
+  // Ensures manual LRU GC for both memory and indexeddb persistence.
+  // In spec tests, GC is always manually triggered via triggerLruGC().
+  ensureManualLruGC(): this {
    debugAssert(
      !this.currentStep,
      'withGCEnabled() must be called before all spec steps.'


Update name in error message

MarkDuckworth · 2023-04-14T13:06:32Z

packages/firestore/test/unit/specs/garbage_collection_spec.test.ts

+          .triggerLruGC(1)
+          .removeExpectedTargetMapping(query1)
+          // should get no events.
+          .userListens(query1)


Is the test name and the test implementation aligned on this one? Maybe I'm not following what it's trying to assert

Nope, fixed.

wu-hui

Thank you!

wu-hui · 2023-04-17T13:48:37Z

packages/firestore/src/api/cache_config.ts

+ *
+ * This collector tries to ensure lowest memory footprints from the SDK,
+ * at the risk of querying backend repeated for a document it could have
+ * cached locally.


wu-hui · 2023-04-17T13:58:02Z

packages/firestore/src/core/component_provider.ts

+
+  createPersistence(cfg: ComponentConfiguration): Persistence {
+    const lruParams =
+      this.cacheSizeBytes !== undefined


This actually won't work because we use -1 to disable GC.

I really do not like this approach, we should do something similar to the new cache config API, using a dedicated enum branch to disable GC. Maybe something for the fixit week.

wu-hui · 2023-04-17T13:58:59Z

packages/firestore/src/core/firestore_client.ts

+     * (incoming GRPC messages), we should always schedule onto this queue. //
+     * This ensures all of our work is properly serialized (e.g. we don't //
+     * start processing a new operation while the previous one is waiting for //
+     * an async I/O to complete). //


Ah, I hate it when my editor does this and i don't know how to stop it.

wu-hui · 2023-04-17T14:05:16Z

packages/firestore/src/local/memory_persistence.ts

+    persistence: MemoryPersistence,
+    lruParams: LruParams | null
+  ): MemoryLruDelegate {
+    return new MemoryLruDelegate(persistence, lruParams!!);


It tells complier that you are sure this is a LruParams, not a null. I removed | null in this case, it was not neccessary.

wu-hui · 2023-04-17T14:07:46Z

packages/firestore/test/unit/specs/garbage_collection_spec.test.ts

+          .triggerLruGC(1)
+          .removeExpectedTargetMapping(query1)
+          // should get no events.
+          .userListens(query1)


Nope, fixed.

wu-hui · 2023-04-17T15:46:16Z

@hsubox76

Please take a look, I need owner's approval for something outside of firestore. I do not understand why they show up though, they seem to be generated automatically.

Check some changes under:

common/
docs-devsite/
packages/auth/api-extractor.json

They are all auth-related.

hsubox76

Sorry about auth.api.md. I made a PR to ensure it's reverted after docgen is run but this branch may have been created before that: Simplify check for reference doc changes #7014 You can just revert it.
Can you add a changeset? It should be @firebase/firestore minor and firebase minor (the yarn changeset CLI should help you add the first automatically, the second has to be added manually)

wu-hui · 2023-04-17T18:59:54Z

Details

Done, thanks!

hsubox76 · 2023-04-17T23:02:31Z

.changeset/olive-cycles-count.md

@@ -0,0 +1,6 @@
+---
+'firebase': patch


These should be minor as it's changing the API.

Fixed, thanks.

hsubox76

LG!

dconeybe · 2023-04-19T01:22:33Z

packages/firestore/test/unit/specs/spec_builder.ts

@@ -1241,6 +1257,6 @@ export function client(
  withGcEnabled?: boolean
 ): MultiClientSpecBuilder {
  const specBuilder = new MultiClientSpecBuilder();
-  specBuilder.withGCEnabled(withGcEnabled === true);
+  specBuilder.ensureManualLruGC();


Shouldn't this be:

if (withGcEnabled !== true) { specBuilder.ensureManualLruGC(); }

Oh yeah, thanks for checking this.

I intended to delete withGcEnabled all together, because multitab will always have lru gc. I just forgot to delete it.

wu-hui added 2 commits January 4, 2023 10:38

Initial implementation

e8b53c6

Add spec tests for LRU GC

3a922f1

lint

107a051

wu-hui force-pushed the wuandy/LruGC branch from c1812df to 107a051 Compare January 11, 2023 20:18

Update API reports

14ed4f3

github-actions bot requested review from dwyfrequency and hsubox76 as code owners January 11, 2023 20:38

github-actions bot added the doc-changes PRs that affect docs label Jan 11, 2023

github-actions bot assigned egilmorez Jan 11, 2023

wu-hui added 3 commits January 12, 2023 12:05

Spec test clean up

7b9d18d

Fix error and polishing.

2ea1fad

Fix a build error

92b1cac

wu-hui force-pushed the wuandy/LruGC branch from 00b690b to 92b1cac Compare January 12, 2023 18:19

wu-hui requested a review from tom-andersen January 13, 2023 18:43

wu-hui assigned tom-andersen and unassigned tom-andersen Jan 13, 2023

wu-hui removed the request for review from tom-andersen January 16, 2023 17:47

egilmorez reviewed Feb 1, 2023

View reviewed changes

packages/firestore/src/api/database.ts Outdated Show resolved Hide resolved

packages/firestore/src/api/database.ts Outdated Show resolved Hide resolved

packages/firestore/src/api/database.ts Outdated Show resolved Hide resolved

egilmorez requested a review from markarndt February 1, 2023 16:35

wu-hui added 6 commits February 8, 2023 14:59

New way to config Firestore SDK Cache.

4ed6691

fixups

48d5077

Fix some test failures

7d0b186

Fixing dts file

d651d88

Fixing integration tests

880bd55

rename file

c36e96f

markarndt approved these changes Feb 28, 2023

View reviewed changes

Increase cache size threshold

85ea128

wu-hui assigned MarkDuckworth and unassigned egilmorez Apr 13, 2023

MarkDuckworth reviewed Apr 14, 2023

View reviewed changes

wu-hui mentioned this pull request Apr 14, 2023

Lru Memory GC firebase/firebase-ios-sdk#10905

Merged

MarkDuckworth approved these changes Apr 14, 2023 •

edited

Loading

View reviewed changes

wu-hui added 4 commits April 17, 2023 10:13

addressing feedback

b83a723

Merge branch 'master' into wuandy/LruGC

6e5767d

docsite

c20f61f

review auth.api.md

68e2f21

wu-hui commented Apr 17, 2023

View reviewed changes

wu-hui mentioned this pull request Apr 17, 2023

LRU Gc for memory firebase/firebase-android-sdk#4724

Merged

Update API reports

5303e16

wu-hui closed this Apr 17, 2023

wu-hui reopened this Apr 17, 2023

wu-hui requested a review from hsubox76 April 17, 2023 15:46

wu-hui assigned hsubox76 Apr 17, 2023

hsubox76 reviewed Apr 17, 2023

View reviewed changes

wu-hui added 2 commits April 17, 2023 14:18

changeset and revert auth changes

5bf4ec8

fix error message

fc4bb55

hsubox76 reviewed Apr 17, 2023

View reviewed changes

minor bump

85483bf

hsubox76 approved these changes Apr 18, 2023

View reviewed changes

wu-hui merged commit 253b998 into master Apr 18, 2023

wu-hui deleted the wuandy/LruGC branch April 18, 2023 18:32

dconeybe reviewed Apr 19, 2023

View reviewed changes

dconeybe mentioned this pull request Apr 19, 2023

Remove gcEnabled for multitab test #7234

Merged

google-oss-bot mentioned this pull request Apr 25, 2023

Version Packages #7266

Merged

firebase locked and limited conversation to collaborators Jun 1, 2023

Memory LRU GC #6943

Memory LRU GC #6943

Conversation

wu-hui commented Jan 11, 2023 • edited Loading

changeset-bot bot commented Jan 11, 2023 • edited Loading

🦋 Changeset detected

google-oss-bot commented Jan 11, 2023 • edited Loading

Size Report 1

Affected Products

@firebase/firestore

bundle

firebase

Test Logs

google-oss-bot commented Jan 11, 2023 • edited Loading

Size Analysis Report 1

Test Logs

egilmorez left a comment

Choose a reason for hiding this comment

MarkDuckworth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-hui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-hui commented Apr 17, 2023

hsubox76 left a comment

Choose a reason for hiding this comment

wu-hui commented Apr 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsubox76 left a comment

Choose a reason for hiding this comment

dconeybe Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wu-hui commented Jan 11, 2023 •

edited

Loading

changeset-bot bot commented Jan 11, 2023 •

edited

Loading

google-oss-bot commented Jan 11, 2023 •

edited

Loading

Size Report ¹

`@firebase/firestore`

`bundle`

`firebase`

google-oss-bot commented Jan 11, 2023 •

edited

Loading

Size Analysis Report ¹

dconeybe Apr 19, 2023 •

edited

Loading