Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not load google-gax at client startup #1517

Merged
merged 5 commits into from
May 27, 2021
Merged

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented May 26, 2021

This improves performance for functions users that only read data from the provided DocumentSnapshot, but do not otherwise access Firestore.

Test file (from @inlined): https://gist.github.com/schmidt-sebastian/f42c30d75785fba436746f894948ecf4

Extra dependencies loaded before PR:

@google-cloud
@grpc
@protobufjs
abort-controller
arrify
base64-js
bignumber.js
buffer-equal-constant-time
duplexify
ecdsa-sig-formatter
end-of-stream
event-target-shim
extend
fast-deep-equal
firebase-admin
functional-red-black-tree
gaxios
gcp-metadata
google-auth-library
google-gax
gtoken
inherits
is-stream
is-stream-ended
json-bigint
jwa
jws
lodash.camelcase
long
lru-cache
node-fetch
node-forge
object-hash
once
protobufjs
readable-stream
retry-request
safe-buffer
stream-shift
util-deprecate
wrappy
yallist

After:

@google-cloud
fast-deep-equal
firebase-admin
node-forge

Cold start time from firebase-functions:

Invocation Branch (ms) Master (ms)
0 1142 3035
1 1034 965
2 743 1080
3 728 728
4 662 738
5 675 851
6 644 791
7 824 763
8 970 716
9 1438 734
     
Average 886 1040
Worst Case 1438 3035

Code:

const functions = require('firebase-functions');

exports.myFunction = functions.firestore
  .document('foo/{docId}')
  .onCreate((change, context) => { 
	  console.info('Executed for document ' + change.id);
	  setTimeout(() => process.exit(0), 1);
});   

@schmidt-sebastian schmidt-sebastian requested review from a team as code owners May 26, 2021 18:12
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 26, 2021
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/nodejs-firestore API. label May 26, 2021
return v1beta1;
},
// scope, we lazy-load the module.
get: () => require('./v1beta1'),

Choose a reason for hiding this comment

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

If require() automatically caches, the modules it loads, why did we cache the modules on this class to begin with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coding is a learning opportunity for all of us.

dev/src/util.ts Outdated
@@ -158,13 +144,28 @@ export function isPermanentRpcError(
}
}

let serviceConfig: undefined | {[k: string]: CallSettings};

Choose a reason for hiding this comment

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

nit: flip undefined to the other side

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

Copy link

@inlined inlined left a comment

Choose a reason for hiding this comment

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

I ran my benchmark agains this branch. The benchmark uses 60 shards which will each try 20 times to purge the require cache and load both the firebase-functions SDK and execute the boilerplate that happens in the cold start for a Firestore function. Here are my results:

stat. vanilla optimized savings
mean 335 253 24%
p50 273 224 18%
p90 513 485 5%
p95 911 589 35%
p99 1498 1150 23%

@schmidt-sebastian schmidt-sebastian merged commit 2141b08 into master May 27, 2021
@schmidt-sebastian schmidt-sebastian deleted the mrschmidt/lazy branch May 27, 2021 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/nodejs-firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants