Skip to content

Remove TargetIdGenerator #2818

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.

Sign up for GitHub

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 6 commits into from
Apr 2, 2020
Merged

Remove TargetIdGenerator #2818

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
09aaec4
Remove TargetIdGenerator
schmidt-sebastian Mar 28, 2020
d251a62
Merge
schmidt-sebastian Mar 31, 2020
9323c9a
Review
schmidt-sebastian Mar 31, 2020
2a9e55e
Trigger CI
schmidt-sebastian Mar 31, 2020
5797b97
Merge/Revert
schmidt-sebastian Apr 1, 2020
69fe8a6
Merge branch 'master' into mrschmidt/removegenerators
schmidt-sebastian Apr 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions packages/firestore/src/core/sync_engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ import { ListenSequence } from './listen_sequence';
import { Query, LimitType } from './query';
import { SnapshotVersion } from './snapshot_version';
import { Target } from './target';
import { TargetIdGenerator } from './target_id_generator';
import { Transaction } from './transaction';
import {
BatchId,
Expand Down Expand Up @@ -161,7 +160,7 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
};
/** Stores user callbacks waiting for all pending writes to be acknowledged. */
private pendingWritesCallbacks = new Map<BatchId, Array<Deferred<void>>>();
private limboTargetIdGenerator = TargetIdGenerator.forSyncEngine();
private nextLimboTargetId = 1;

// The primary state is set to `true` or `false` immediately after Firestore
// startup. In the interim, a client should only be considered primary if
Expand Down Expand Up @@ -770,7 +769,11 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
const key = limboChange.key;
if (!this.limboTargetsByKey.get(key)) {
logDebug(LOG_TAG, 'New document in limbo: ' + key);
const limboTargetId = this.limboTargetIdGenerator.next();

// Target IDs in SyncEngine start at 1 and remain odd.
const limboTargetId = this.nextLimboTargetId;
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that was nice about the separate class was that it fully encapsulated this post-increment semantic (that is: get the current value and then bump). Now different locations are using this differently and it's not obvious how this is supposed to work.

In particular, it's a little weird that MemoryTargetCache doesn't use the appropriate TARGET_ID initializer and doesn't do the post-increment dance, though I guess that's sort of intended?

It's also telling that the client state implements this as a method.

In several cases in this review I had to work to convince myself that this was going to generate the correct initial value and this is uncomfortable.

I bet you'd get most of the size benefit (possibly a bit more), just by simplifying the internals, having a constructor that just takes the initial values you've exported, and possibly removing seek/after and having those callers just new up a new TargetIdGenerator with their desired seed.

this.nextLimboTargetId += 2;

const query = Query.atPath(key.path);
this.limboResolutionsByTarget[limboTargetId] = new LimboResolution(key);
this.remoteStore.listen(
Expand Down
97 changes: 0 additions & 97 deletions packages/firestore/src/core/target_id_generator.ts
View file
Open in desktop

This file was deleted.

10 changes: 3 additions & 7 deletions packages/firestore/src/local/indexeddb_target_cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -23,7 +23,6 @@ import { DocumentKey } from '../model/document_key';
import { assert } from '../util/assert';
import { immediateSuccessor } from '../util/misc';

import { TargetIdGenerator } from '../core/target_id_generator';
import * as EncodedResourcePath from './encoded_resource_path';
import {
IndexedDbLruDelegate,
Expand Down Expand Up @@ -60,15 +59,12 @@ export class IndexedDbTargetCache implements TargetCache {
// to IndexedDb whenever we need to read metadata. We can revisit if it turns
// out to have a meaningful performance impact.

private targetIdGenerator = TargetIdGenerator.forTargetCache();

allocateTargetId(
transaction: PersistenceTransaction
): PersistencePromise<TargetId> {
return this.retrieveMetadata(transaction).next(metadata => {
metadata.highestTargetId = this.targetIdGenerator.after(
metadata.highestTargetId
);
// Target IDs in persistence start at two and remain even.
metadata.highestTargetId += 2;
return this.saveMetadata(transaction, metadata).next(
() => metadata.highestTargetId
);
Expand Down
11 changes: 4 additions & 7 deletions packages/firestore/src/local/memory_target_cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -16,7 +16,6 @@
*/

import { SnapshotVersion } from '../core/snapshot_version';
import { TargetIdGenerator } from '../core/target_id_generator';
import { ListenSequenceNumber, TargetId } from '../core/types';
import { DocumentKeySet } from '../model/collections';
import { DocumentKey } from '../model/document_key';
Expand Down Expand Up @@ -52,8 +51,6 @@ export class MemoryTargetCache implements TargetCache {

private targetCount = 0;

private targetIdGenerator = TargetIdGenerator.forTargetCache();

constructor(private readonly persistence: MemoryPersistence) {}

forEachTarget(
Expand All @@ -79,9 +76,9 @@ export class MemoryTargetCache implements TargetCache {
allocateTargetId(
transaction: PersistenceTransaction
): PersistencePromise<TargetId> {
const nextTargetId = this.targetIdGenerator.after(this.highestTargetId);
this.highestTargetId = nextTargetId;
return PersistencePromise.resolve(nextTargetId);
// Target IDs in persistence start at two and remain even.
this.highestTargetId += 2;
return PersistencePromise.resolve(this.highestTargetId);
}

setTargetsMetadata(
Expand Down
62 changes: 0 additions & 62 deletions packages/firestore/test/unit/core/target_id_generator.test.ts
View file
Open in desktop

This file was deleted.

23 changes: 14 additions & 9 deletions packages/firestore/test/unit/specs/spec_builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

import { FieldFilter, Filter, Query } from '../../../src/core/query';
import { Target } from '../../../src/core/target';
import { TargetIdGenerator } from '../../../src/core/target_id_generator';
import { TargetId } from '../../../src/core/types';
import {
Document,
Expand Down Expand Up @@ -82,8 +81,7 @@ export class ClientMemoryState {
activeTargets: ActiveTargetMap = {};
queryMapping = new ObjectMap<Target, TargetId>(t => t.canonicalId());
limboMapping: LimboMap = {};

limboIdGenerator: TargetIdGenerator = TargetIdGenerator.forSyncEngine();
nextLimboTarget = 1;

constructor() {
this.reset();
Expand All @@ -94,7 +92,13 @@ export class ClientMemoryState {
this.queryMapping = new ObjectMap<Target, TargetId>(t => t.canonicalId());
this.limboMapping = {};
this.activeTargets = {};
this.limboIdGenerator = TargetIdGenerator.forSyncEngine();
this.nextLimboTarget = 1;
}

nextLimboTargetId(): TargetId {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Naming the nextLimboTargetId() method the same as the variable name somewhat implies that it is a simple "getter" method. Consider renaming to something that implies that it is "doing something" not just "getting something". e.g. generateLimboTargetId().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to generateLimboTargetId.

const nextLimboTargetId = this.nextLimboTarget;
this.nextLimboTarget += 2;
return nextLimboTargetId;
}

/**
Expand All @@ -112,7 +116,7 @@ export class ClientMemoryState {
class CachedTargetIdGenerator {
// TODO(wuandy): rename this to targetMapping.
private queryMapping = new ObjectMap<Target, TargetId>(t => t.canonicalId());
private targetIdGenerator = TargetIdGenerator.forTargetCache();
private nextTargetId = 2;

/**
* Returns a cached target ID for the provided Target, or a new ID if no
Expand All @@ -122,7 +126,8 @@ class CachedTargetIdGenerator {
if (this.queryMapping.has(target)) {
return this.queryMapping.get(target)!;
}
const targetId = this.targetIdGenerator.next();
const targetId = this.nextTargetId;
this.nextTargetId += 2;
this.queryMapping.set(target, targetId);
return targetId;
}
Expand Down Expand Up @@ -170,8 +175,8 @@ export class SpecBuilder {
return this.currentClientState;
}

private get limboIdGenerator(): TargetIdGenerator {
return this.clientState.limboIdGenerator;
private nextLimboTargetId(): TargetId {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here; consider renaming the nextLimboTargetId() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return this.clientState.nextLimboTargetId();
}

private get queryMapping(): ObjectMap<Target, TargetId> {
Expand Down Expand Up @@ -450,7 +455,7 @@ export class SpecBuilder {
const path = key.path.canonicalString();
// Create limbo target ID mapping if it was not in limbo yet
if (!objUtils.contains(this.limboMapping, path)) {
this.limboMapping[path] = this.limboIdGenerator.next();
this.limboMapping[path] = this.nextLimboTargetId();
}
// Limbo doc queries are always without resume token
this.addQueryToActiveTargets(
Expand Down