Memory LRU GC by wu-hui · Pull Request #6943 · firebase/firebase-js-sdk (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation31 Commits46 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Googlers see: go/firestore-memory-lru
Size Report 1
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
- Base (1d6771e): https://github.com/firebase/firebase-js-sdk/actions/runs/4704785514
- Merge (7a96fce): https://github.com/firebase/firebase-js-sdk/actions/runs/4731777444
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few tweaks to the public doc comments, thanks!
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with comments
| * |
|---|
| * 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. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| createPersistence(cfg: ComponentConfiguration): Persistence { |
| const lruParams = |
| this.cacheSizeBytes !== undefined |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if we need a range check for this.cacheSizeBytes > 0
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| * (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). // |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if ending // were intentional
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I hate it when my editor does this and i don't know how to stop it.
| persistence: MemoryPersistence, |
|---|
| lruParams: LruParams | null |
| ): MemoryLruDelegate { |
| return new MemoryLruDelegate(persistence, lruParams!!); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the double exclamation at the end of lruParams!! do? I'm not familiar with the double.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It tells complier that you are sure this is a LruParams, not a null. I removed | null in this case, it was not neccessary.
| ensureManualLruGC(): this { |
|---|
| debugAssert( |
| !this.currentStep, |
| 'withGCEnabled() must be called before all spec steps.' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update name in error message
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| .triggerLruGC(1) |
|---|
| .removeExpectedTargetMapping(query1) |
| // should get no events. |
| .userListens(query1) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the test name and the test implementation aligned on this one? Maybe I'm not following what it's trying to assert
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, fixed.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
| * |
|---|
| * 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. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| createPersistence(cfg: ComponentConfiguration): Persistence { |
| const lruParams = |
| this.cacheSizeBytes !== undefined |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| * (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). // |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I hate it when my editor does this and i don't know how to stop it.
| persistence: MemoryPersistence, |
|---|
| lruParams: LruParams | null |
| ): MemoryLruDelegate { |
| return new MemoryLruDelegate(persistence, lruParams!!); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It tells complier that you are sure this is a LruParams, not a null. I removed | null in this case, it was not neccessary.
| .triggerLruGC(1) |
|---|
| .removeExpectedTargetMapping(query1) |
| // should get no events. |
| .userListens(query1) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, fixed.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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: #7014 You can just revert it.
- Can you add a changeset? It should be
@firebase/firestoreminor andfirebaseminor (theyarn changesetCLI should help you add the first automatically, the second has to be added manually)
| @@ -0,0 +1,6 @@ |
|---|
| --- |
| 'firebase': patch |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be minor as it's changing the API.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG!
| ): MultiClientSpecBuilder { |
|---|
| const specBuilder = new MultiClientSpecBuilder(); |
| specBuilder.withGCEnabled(withGcEnabled === true); |
| specBuilder.ensureManualLruGC(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be:
if (withGcEnabled !== true) { specBuilder.ensureManualLruGC(); }
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
