Skip to content

Use approximate FieldValue size for MemoryLRU #2548

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 5 commits into from
Jan 22, 2020
Merged

Use approximate FieldValue size for MemoryLRU #2548

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c0a86af
Use approximate FieldValue size for MemoryLRU
schmidt-sebastian Jan 16, 2020
e6a786f
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Jan 16, 2020
300c9bd
Review
schmidt-sebastian Jan 17, 2020
6b83a8a
Merge branch 'master' into mrschmidt/byte
schmidt-sebastian Jan 21, 2020
9f756d4
Feedback
schmidt-sebastian Jan 22, 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
26 changes: 6 additions & 20 deletions packages/firestore/src/local/memory_persistence.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,13 @@
*/

import { User } from '../auth/user';
import { MaybeDocument } from '../model/document';
import { Document, MaybeDocument } from '../model/document';
import { DocumentKey } from '../model/document_key';
import { JsonProtoSerializer } from '../remote/serializer';
import { fail } from '../util/assert';
import { debug } from '../util/log';
import * as obj from '../util/obj';
import { ObjectMap } from '../util/obj_map';
import { encode } from './encoded_resource_path';
import { LocalSerializer } from './local_serializer';
import {
ActiveTargets,
LruDelegate,
Expand Down Expand Up @@ -77,11 +75,10 @@ export class MemoryPersistence implements Persistence {

static createLruPersistence(
clientId: ClientId,
serializer: JsonProtoSerializer,
params: LruParams
): MemoryPersistence {
const factory = (p: MemoryPersistence): MemoryLruDelegate =>
new MemoryLruDelegate(p, new LocalSerializer(serializer), params);
new MemoryLruDelegate(p, params);
return new MemoryPersistence(clientId, factory);
}

Expand Down Expand Up @@ -334,7 +331,6 @@ export class MemoryLruDelegate implements ReferenceDelegate, LruDelegate {

constructor(
private readonly persistence: MemoryPersistence,
private readonly serializer: LocalSerializer,
lruParams: LruParams
) {
this.garbageCollector = new LruGarbageCollector(this, lruParams);
Expand Down Expand Up @@ -471,21 +467,11 @@ export class MemoryLruDelegate implements ReferenceDelegate, LruDelegate {
}

documentSize(maybeDoc: MaybeDocument): number {
const remoteDocument = this.serializer.toDbRemoteDocument(
maybeDoc,
maybeDoc.version
);
let value: unknown;
if (remoteDocument.document) {
value = remoteDocument.document;
} else if (remoteDocument.unknownDocument) {
value = remoteDocument.unknownDocument;
} else if (remoteDocument.noDocument) {
value = remoteDocument.noDocument;
} else {
throw fail('Unknown remote document type');
let documentSize = maybeDoc.key.toString().length;
if (maybeDoc instanceof Document) {
documentSize += maybeDoc.data().byteSize();
}
return JSON.stringify(value).length;
return documentSize;
}

private isPinned(
Expand Down
69 changes: 69 additions & 0 deletions packages/firestore/src/model/field_value.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,12 @@ export abstract class FieldValue {
abstract isEqual(other: FieldValue): boolean;
abstract compareTo(other: FieldValue): number;

/**
* Returns an approximate (and wildly inaccurate) in-memory size for the field
* value.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's worth specifying that implementations:

  • should be guided by some standard assumptions (i.e. in-memory proto representation, not VM representation; ignoring variable-length encoding)
  • Should only account for user data, not any object overhead

The first point is important because there are some elements where the JSON or proto representations that are both right and wrong for this purpose. For example:

  • JSON encodes numbers in their textual form, but calculating this length is actually pretty expensive
  • protobuf uses varint encoding, wherein an int64 values from -64 to 63 encode as a single byte
  • Javascript declares boolean values to occupy a single byte
  • VM differences also shouldn't matter: i.e. in JavaScript, strings are UCS-2 or UTF-16, but in C++ (for std::string at least) they're UTF-8. Ideally we should define this in a way that's portable to all these implementations.

Having some consistent frame of reference for this will make it easier to understand if the values we're using are reasonable.

The second part is important because the full memory size doesn't actually matter. We can make representation choices (e.g. boxed value or not). This matters more in languages like Java, where it's more explicit, or C++ where it's front and center, but I think we should explicitly delineate that boxing/class overhead should be ignored. This also helps make this implementation portable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as this is only used in Memory LRU, we should at least attempt to replicate the in-memory cost of storing a document, which makes the VM representation the biggest driving factor. I know that this implementation is already widely off (and probably aggressively undercounts), but I personally don't think that the implementation should be driven by platform convergence, JSON or Protobuf sizes, or any on-disk presentation. If we can, we should optimize for the user experience - if the memory LRU limit is set to 40 MBs, Firestore should try to approximate this.

With that said, I updated the comment to specify that it only accounts for user data and ignores object overhead.

*/
abstract byteSize(): number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming approximateByteSize() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was torn between "byteSize()", "estimateByteSize()" and "approximateByteSize()". Looks like we have two votes for approximateByteSize(). Changed.


toString(): string {
const val = this.value();
return val === null ? 'null' : val.toString();
Expand Down Expand Up @@ -167,6 +173,10 @@ export class NullValue extends FieldValue {
return this.defaultCompareTo(other);
}

byteSize(): number {
return 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me why you chose 1. I'm not saying it's a bad value, but that it might be good for us to be deliberate about our approximations and document them to make it clearer the intention and allow some future person to improve them.

It's tempting to try to be consistent with the old code... in which case, would this use 4 since the JSON-encoding is null which is 4 bytes?

Alternatively, if this is really meant to represent the in-memory representation, I would guess (but could be wrong that at minimum, JavaScript stores values at 4-byte-aligned locations, so the minimum size of anything is probably 4 bytes.

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Jan 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I mostly chose 1 because I didn't want a document with a bunch of NULLs to be zero. 4 sounds reasonable, and basing it on the previous JSON-encoded size makes it even seem super scientific and well thought-out. If only the rest of this PR was more like this :)

}

static INSTANCE = new NullValue();
}

Expand Down Expand Up @@ -195,6 +205,10 @@ export class BooleanValue extends FieldValue {
return this.defaultCompareTo(other);
}

byteSize(): number {
return 1;
}

static of(value: boolean): BooleanValue {
return value ? BooleanValue.TRUE : BooleanValue.FALSE;
}
Expand All @@ -221,6 +235,10 @@ export abstract class NumberValue extends FieldValue {
}
return this.defaultCompareTo(other);
}

byteSize(): number {
return 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be 8? (double / long are 8)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Yes.

}
}

/** Utility function to compare doubles (using Firestore semantics for NaN). */
Expand Down Expand Up @@ -313,6 +331,13 @@ export class StringValue extends FieldValue {
}
return this.defaultCompareTo(other);
}

byteSize(): number {
// See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures:
// "JavaScript's String type is [...] a set of elements of 16-bit unsigned
// integer values"
return this.internalValue.length * 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that if we're going for the on-disk size, this will probably (not 100% sure about IndexedDb) be UTF-8 encoded which means most characters will be 1-byte, not 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mostly (at least for now) for memory size. IndexedDB size accounting also uses JSON.stringify(), but on already available Protobuf data.

Copy link
Contributor

Choose a reason for hiding this comment

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

Memory size isn't a super great proxy because it's not portable. Strings in C++ are encoded in UTF-8.

Would it make sense to define this as UTF-8-encoded size and then just approximate by saying that on average characters encode to 1.1 bytes?

I'm not sure this really matters, but it seems like a portable, single definition of what these sizes are would be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strings are the only primitive with a specified memory footprint on https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures. I am ok with "dumbing this down" and just using 1 byte, but as eluded to above, I don't think aiming for consistency among platforms is a worthy goal.

}
}

export class TimestampValue extends FieldValue {
Expand Down Expand Up @@ -347,6 +372,11 @@ export class TimestampValue extends FieldValue {
return this.defaultCompareTo(other);
}
}

byteSize(): number {
// Timestamps are made up of two distinct numbers (seconds + nanoseconds)
return 8;
}
}

/**
Expand Down Expand Up @@ -410,6 +440,13 @@ export class ServerTimestampValue extends FieldValue {
toString(): string {
return '<ServerTimestamp localTime=' + this.localWriteTime.toString() + '>';
}

byteSize(): number {
return (
/* localWriteTime */ 8 +
(this.previousValue ? this.previousValue.byteSize() : 0)
);
}
}

export class BlobValue extends FieldValue {
Expand All @@ -436,6 +473,10 @@ export class BlobValue extends FieldValue {
}
return this.defaultCompareTo(other);
}

byteSize(): number {
return this.internalValue.toUint8Array().byteLength;
Copy link
Contributor

Choose a reason for hiding this comment

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

All of the other calculations are cheap... should we add a byteSize to Blob?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To reduce size one must first increase size.

Done.

}
}

export class RefValue extends FieldValue {
Expand Down Expand Up @@ -466,6 +507,14 @@ export class RefValue extends FieldValue {
}
return this.defaultCompareTo(other);
}

byteSize(): number {
return (
this.databaseId.projectId.length +
this.databaseId.database.length +
this.key.toString().length
);
}
}

export class GeoPointValue extends FieldValue {
Expand All @@ -492,6 +541,11 @@ export class GeoPointValue extends FieldValue {
}
return this.defaultCompareTo(other);
}

byteSize(): number {
// GeoPoints are made up of two distinct numbers (latitude + longitude)
return 8;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should perhaps be 16 then, I think... although JavaScript may use some clever encoding of numbers that allows it to use 4-bytes for small numbers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this, Timestamps and ServerTimestamps 16.

}
}

export class ObjectValue extends FieldValue {
Expand Down Expand Up @@ -634,6 +688,14 @@ export class ObjectValue extends FieldValue {
return FieldMask.fromSet(fields);
}

byteSize(): number {
let size = 0;
this.internalValue.inorderTraversal((key, val) => {
size += key.length + val.byteSize();
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with your other code, this would be 2* key.length, right? But again, if we assume UTF-8 encoding, then this is okay.

FWIW it is tempting to create a Sizer helper class that consistently deals with data types. E.g. Sizer.string(key.length) + val.byteSize()

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Jan 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I was debating all of this too... but I think we are already widely off with our size accounting and keeping it simple has some value. I kept it as is for now (including the inconsistency with StringValue, which at least has a comment).

BTW, the same issue applies to ReferenceValues. We could add a Sizer class but that means more code and more cross-platform inconsistencies.

});
return size;
}

toString(): string {
return this.internalValue.toString();
}
Expand Down Expand Up @@ -720,6 +782,13 @@ export class ArrayValue extends FieldValue {
}
}

byteSize(): number {
return this.internalValue.reduce(
(previousSize, value) => previousSize + value.byteSize(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Would previousSize => totalSize or accumulatedSize be an improvement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Picked totalSize.

0
);
}

toString(): string {
const descriptions = this.internalValue.map(v => v.toString());
return `[${descriptions.join(',')}]`;
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/test/integration/api/database.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,7 +814,7 @@ apiDescribe('Database', (persistence: boolean) => {
// eslint-disable-next-line no-restricted-properties
describe.skip('Listens are rejected remotely:', () => {
//eslint-disable-next-line @typescript-eslint/no-explicit-any
const queryForRejection : firestore.Query = null as any;
const queryForRejection: firestore.Query = null as any;

it('will reject listens', () => {
const deferred = new Deferred();
Expand Down
6 changes: 1 addition & 5 deletions packages/firestore/test/unit/local/persistence_test_helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,7 @@ export async function testMemoryEagerPersistence(): Promise<MemoryPersistence> {
export async function testMemoryLruPersistence(
params: LruParams = LruParams.DEFAULT
): Promise<MemoryPersistence> {
return MemoryPersistence.createLruPersistence(
AutoId.newId(),
JSON_SERIALIZER,
params
);
return MemoryPersistence.createLruPersistence(AutoId.newId(), params);
}

/** Clears the persistence in tests */
Expand Down
54 changes: 54 additions & 0 deletions packages/firestore/test/unit/model/field_value.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { expect } from 'chai';
import { GeoPoint } from '../../../src/api/geo_point';
import { Timestamp } from '../../../src/api/timestamp';
import * as fieldValue from '../../../src/model/field_value';
import { primitiveComparator } from '../../../src/util/misc';
import * as typeUtils from '../../../src/util/types';
import {
blob,
Expand Down Expand Up @@ -485,4 +486,57 @@ describe('FieldValue', () => {
}
);
});

it('estimates size correctly for fixed sized values', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests are pretty clever in that they don't encode the actual sizes, but they don't directly verify the implementation. It would be more straightforward if the tests just listed out:

function sizeof(value) {
  return wrap(value).approximateByteSize();
}

it('estimates size correctly for fixed sized values', () => {
  expect(sizeof(null)).to.be(4);
  expect(sizeof('')).to.be(0);
  // etc ...

});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an explicit expectedByteSize.

// This test verifies that each member of a group takes up the same amount
// of space in memory (based on its estimated in-memory size).
const equalityGroups = [
[wrap(null), wrap(false), wrap(true)],
[wrap(blob(0, 1)), wrap(blob(128, 129))],
[wrap(NaN), wrap(Infinity), wrap(1), wrap(1.1)],
[wrap(new GeoPoint(0, 0)), wrap(new GeoPoint(0, 0))],
[wrap(Timestamp.fromMillis(100)), wrap(Timestamp.now())],
[
new fieldValue.ServerTimestampValue(Timestamp.fromMillis(100), null),
new fieldValue.ServerTimestampValue(Timestamp.now(), null)
]
];

for (const group of equalityGroups) {
const expectedItemSize = group[0].byteSize();
for (const element of group) {
expect(element.byteSize()).to.equal(expectedItemSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth verifying/possible to verify that no size is zero?

I'm a little worried that these tests don't actually verify that we're aggregating the sizes of objects correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should now be taken care of by explicitly comparing the size to a provided value. I also added more types to this test case.

}
}
});

it('estimates size correctly for relatively sized values', () => {
// This test verifies for each group that the estimated size increases
// as the size of the underlying data grows.
Copy link
Contributor

Choose a reason for hiding this comment

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

Clever!

const relativeGroups = [
[wrap(blob(0)), wrap(blob(0, 1))],
[
new fieldValue.ServerTimestampValue(Timestamp.fromMillis(100), null),
new fieldValue.ServerTimestampValue(Timestamp.now(), wrap(null))
],
[
new fieldValue.RefValue(dbId('p1', 'd1'), key('c1/doc1')),
new fieldValue.RefValue(dbId('p1', 'd1'), key('c1/doc1/c2/doc2'))
],
[wrap('foo'), wrap('foobar')],
[wrap(['a', 'b']), wrap(['a', 'bc'])],
[wrap(['a', 'b']), wrap(['a', 'b', 'c'])],
[wrap({ a: 'a', b: 'b' }), wrap({ a: 'a', b: 'bc' })],
[wrap({ a: 'a', b: 'b' }), wrap({ a: 'a', bc: 'b' })],
[wrap({ a: 'a', b: 'b' }), wrap({ a: 'a', b: 'b', c: 'c' })]
];

for (const group of relativeGroups) {
const expectedOrder = group;
const actualOrder = group
.slice()
.sort((l, r) => primitiveComparator(l.byteSize(), r.byteSize()));
expect(expectedOrder).to.deep.equal(actualOrder);
}
});
});
1 change: 0 additions & 1 deletion packages/firestore/test/unit/specs/spec_test_runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1238,7 +1238,6 @@ class MemoryTestRunner extends TestRunner {
? MemoryPersistence.createEagerPersistence(this.clientId)
: MemoryPersistence.createLruPersistence(
this.clientId,
serializer,
LruParams.DEFAULT
)
);
Expand Down