Skip to content

Conversation

@schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented May 19, 2018

This PR is a fork of #196 and addresses a performance issue on Google Cloud Functions. Firestore usage on GCF is often read-only - that is, they provide the payload for a DocumentSnapshot and use Firestore only to deserialize this payload. We don't need to initialize the networking stack unless the user ends up writing back to Firestore.

This PR lazy-loads v1beta1 and @google-cloud/common.

The PR also removes @google-cloud/common-grpc (as this also pulls in the GRPC dependency). Unfortunately, this means that we no longer inherit from GrpcService. Is that a problem?

Test code:

const start = Date.now();
var Firestore = require('@google-cloud/firestore');
var snapshot = new Firestore().snapshot_('projects/foo/databases/bar/documents/coll/missing', {});
const end = Date.now();
console.log('Load time:', (end-start), 'ms');

Before: 473 ms, After: 34ms on a fully beefed-up Macbook

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 19, 2018
@codecov
Copy link

codecov bot commented May 19, 2018

Codecov Report

Merging #197 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #197   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines        1801   1800    -1     
=====================================
- Hits         1801   1800    -1
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54e3ca4...1e26750. Read the comment docs.

@alexander-fenster
Copy link
Contributor

I'm not the one making the call here - @crwilcox is responsible for the libraries and @stephenplusplus and @callmehiphop support the common package.

@alexander-fenster alexander-fenster requested review from callmehiphop, crwilcox and stephenplusplus and removed request for alexander-fenster May 19, 2018 00:14
@stephenplusplus
Copy link
Contributor

The features of common-grpc/service & common-grpc/service-object are:

  • Detect authentication
  • Intercept all requests (including streams) to:
    • handle exponential backoff retries
    • allow auto-detection of project ID, replacing {{projectId}} placeholders
  • Define common methods (delete, get, getMetadata, setMetadata)

If you're not using these things, then it should be safe to remove.

@schmidt-sebastian
Copy link
Contributor Author

I confirmed that we are not using any of these features (by adding breakpoints), since all of our requests go through the Veneer layer. I have also tested a patched client that includes these changes and am able to write and read documents.

@schmidt-sebastian schmidt-sebastian merged commit 1f2f525 into master May 23, 2018
@schmidt-sebastian schmidt-sebastian deleted the inlined-lazyload branch July 24, 2018 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants