Remove TargetIdGenerator #2818

schmidt-sebastian · 2020-03-28T00:28:01Z

This PR removes the TargetIdGenerator, which offered a fancy way to add +2 to integers.

google-oss-bot · 2020-03-28T00:48:03Z

Binary Size Report

Affected SDKs

SDK	Type	Base (`7b17e03`)	Head (`49624d6`)	Diff
@firebase/firestore/memory	browser	212770.00	212492.00	-278.00 (-0.13%)
	module	211136.00	210858.00	-278.00 (-0.13%)
	esm2017	169785.00	169548.00	-237.00 (-0.14%)
	main	376777.00	376232.00	-545.00 (-0.14%)
@firebase/firestore	browser	269110.00	268827.00	-283.00 (-0.11%)
	module	267081.00	266798.00	-283.00 (-0.11%)
	esm2017	215496.00	215260.00	-236.00 (-0.11%)
	main	487448.00	486884.00	-564.00 (-0.12%)
firebase	firebase.js	846328.00	846043.00	-285.00 (-0.03%)
	firebase-firestore.memory.js	256172.00	255892.00	-280.00 (-0.11%)
	firebase-firestore.js	311240.00	310955.00	-285.00 (-0.09%)

Metric Unit: byte

Test Logs

dconeybe

IMO this change detracts from readability and adds logic duplication throughout the code base. It looked nicer to me that the integer increment by 2 was abstracted away into a TargetIdGenerator class instead of having to be explicitly mentioned elsewhere. It also (in theory) would have allowed for the TargetIdGenerator class to be properly unit tested. Could you provide a bit of explanation as to your reasoning for this change? Does it have to do with tree shaking? Thanks!

dconeybe · 2020-03-30T13:46:12Z

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

+    this.nextLimboTarget = 1;
+  }
+
+  nextLimboTargetId(): TargetId {


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().

Renamed to generateLimboTargetId.

dconeybe · 2020-03-30T13:46:56Z

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

@@ -170,8 +175,8 @@ export class SpecBuilder {
    return this.currentClientState;
  }

-  private get limboIdGenerator(): TargetIdGenerator {
-    return this.clientState.limboIdGenerator;
+  private nextLimboTargetId(): TargetId {


Same here; consider renaming the nextLimboTargetId() method.

schmidt-sebastian

I updated the PR a bit to address some of your concerns. I ended up factoring out the "+2" logic into its own function.
TargetIdGenerator was always a bit overblown (it even used to use bitmasks) and together with its tests it added quite a bit of code to our codebase for what is essentially a very simple sequence number generator.

schmidt-sebastian · 2020-03-31T15:19:22Z

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

+    this.nextLimboTarget = 1;
+  }
+
+  nextLimboTargetId(): TargetId {


Renamed to generateLimboTargetId.

schmidt-sebastian · 2020-03-31T15:19:31Z

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

@@ -170,8 +175,8 @@ export class SpecBuilder {
    return this.currentClientState;
  }

-  private get limboIdGenerator(): TargetIdGenerator {
-    return this.clientState.limboIdGenerator;
+  private nextLimboTargetId(): TargetId {


dconeybe

LGTM. Thank you for factoring out the +2 logic.

wilhuff · 2020-03-31T21:25:04Z

packages/firestore/src/core/target.ts

+/** Target IDs for Limbo resolution are odd. */
+export const TARGET_ID_OFFSET_SYNC_ENGINE: TargetId = 1;
+
+/** Target IDs for Queries start are even. */


This comment doesn't clearly explain why the first value is 2, when 0 seems like a reasonable first value for even numbers. The comment in target_id_generator is worth preserving:

// We seed the query cache generator to return '2' as its first ID, as there // is no differentiation in the protocol layer between an unset number and // the number '0'. If we were to sent a target with target ID '0', the // backend would consider it unset and replace it with its own ID.

wilhuff · 2020-03-31T21:30:06Z

packages/firestore/src/core/target.ts

@@ -153,3 +154,20 @@ export class Target {
    );
  }
 }
+
+/** Target IDs for Limbo resolution are odd. */
+export const TARGET_ID_OFFSET_SYNC_ENGINE: TargetId = 1;


These aren't "offsets" so much as "initial values". How about INITIAL_TARGET_ID_FOR_SYNC_ENGINE or SYNC_ENGINE_INITIAL_TARGET_ID?

Also, for whatever reason we spell BATCHID_UNKNOWN with no underscore before ID. Let's make these consistent.

That name also puts BATCHID first so maybe this should be TARGETID_INITIAL_FOR_SYNC_ENGINE or TARGETID_SYNC_ENGINE_INITIAL?

Gah! Sorry for the bike shed.

wilhuff · 2020-03-31T21:47:39Z

packages/firestore/src/core/sync_engine.ts

@@ -809,7 +808,8 @@ export class SyncEngine implements RemoteSyncer, SharedClientStateSyncer {
      this.activeLimboTargetsByKey.size < this.maxConcurrentLimboResolutions
    ) {
      const key = this.enqueuedLimboResolutions.shift()!;
-      const limboTargetId = this.limboTargetIdGenerator.next();
+      const limboTargetId = this.nextLimboTargetId;


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.

schmidt-sebastian

Updated the PR to revert the removal of the generator class. I simplified the class, which still shaves off a tiny bit of size and a fair bit of mental overhead.

Nonetheless, I am still not 100% convinced that we need our own type, especially since the complicated part of the PR was that I had to manually ensure that the Target IDs generated by the Spec Tests and by the SDK itself match. By sharing the generator logic, we now again rely on the generators to verify their own target IDs.

Either way, I don't feel strongly and spending another hour to shave off 100 bytes is definitely not worth it :)

wilhuff · 2020-04-02T18:40:00Z

packages/firestore/src/core/target_id_generator.ts

-      'Cannot supply target ID from different generator ID'
-    );
-    this.nextId = targetId;
+    this.lastId += OFFSET;


This is functionally different from what came before. This is now a pre-increment where before it was post-increment. Just double-checking: is this intended?

Yes. The target ID generator now gets initialized with 0 and -1 (shudder), which works around this inconsistency.

wilhuff

LGTM

schmidt-sebastian force-pushed the mrschmidt/removegenerators branch from e5d7003 to fca4320 Compare March 28, 2020 03:00

Remove TargetIdGenerator

09aaec4

schmidt-sebastian force-pushed the mrschmidt/removegenerators branch from c3df99e to 09aaec4 Compare March 29, 2020 20:09

schmidt-sebastian requested a review from dconeybe March 29, 2020 20:27

schmidt-sebastian assigned dconeybe Mar 29, 2020

schmidt-sebastian added the api: firestore label Mar 29, 2020

dconeybe reviewed Mar 30, 2020

View reviewed changes

dconeybe assigned schmidt-sebastian and unassigned dconeybe Mar 30, 2020

schmidt-sebastian added 3 commits March 31, 2020 08:10

Merge

d251a62

Review

9323c9a

Trigger CI

2a9e55e

schmidt-sebastian commented Mar 31, 2020

View reviewed changes

schmidt-sebastian assigned dconeybe and unassigned schmidt-sebastian Mar 31, 2020

dconeybe approved these changes Mar 31, 2020

View reviewed changes

dconeybe assigned schmidt-sebastian and unassigned dconeybe Mar 31, 2020

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Mar 31, 2020

wilhuff reviewed Apr 1, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Apr 1, 2020

schmidt-sebastian force-pushed the mrschmidt/removegenerators branch from 34e3971 to e523c99 Compare April 1, 2020 05:24

Merge/Revert

5797b97

schmidt-sebastian force-pushed the mrschmidt/removegenerators branch from e523c99 to 5797b97 Compare April 1, 2020 05:27

schmidt-sebastian commented Apr 1, 2020

View reviewed changes

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Apr 1, 2020

wilhuff reviewed Apr 2, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Apr 2, 2020

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Apr 2, 2020

wilhuff approved these changes Apr 2, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Apr 2, 2020

Merge branch 'master' into mrschmidt/removegenerators

69fe8a6

schmidt-sebastian merged commit d69f79c into master Apr 2, 2020

schmidt-sebastian deleted the mrschmidt/removegenerators branch April 2, 2020 23:36

firebase locked and limited conversation to collaborators May 3, 2020

Remove TargetIdGenerator #2818

Remove TargetIdGenerator #2818

Uh oh!

Conversation

schmidt-sebastian commented Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented Mar 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

Test Logs

Uh oh!

dconeybe left a comment

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!

schmidt-sebastian left a comment

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!

dconeybe left a comment

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!

schmidt-sebastian left a comment

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!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schmidt-sebastian commented Mar 28, 2020 •

edited

Loading

google-oss-bot commented Mar 28, 2020 •

edited

Loading