Improve network latency of runTransaction() routine

[brettwillis]

If I correctly understand the routine for runTransaction(), as referenced below, then when running a read-write transaction, a documents:beginTransaction request is always issued before running the updateFn.

nodejs-firestore/dev/src/transaction.ts

Lines 593 to 598 in e598b9d

	async runTransactionOnce<T>(
	updateFunction: (transaction: Transaction) => Promise<T>
	): Promise<T> {
	if (!this._readTime) {
	await this.begin();
	}

This means we wait for an entire network round trip before beginning the transaction, every attempt.

I also noticed that the documents:runQuery and documents:batchGet APIs support a newTransaction parameter, which would allow the SDK to lazily start a transaction on the first read request, without the additional sequential network round trip.

Lazily starting the transaction on the first read would always eliminate one entire sequential network round trip for every read-write transaction. I feel that this is quite a significant impact for high performance applications.

Some implications

• The transaction would be started server-side on first read (eliminate documents:beginTransaction and use newTransaction).
• All read operations would need to use documents:batchGet, documents:runQuery, documents: runAggregationQuery (i.e. not documents:get) which is fine.
• Subsequent reads would be queued until the first read completes, so as to use that same transaction ID returned. In many cases, transaction reads would be sequential anyways, and if not, the user can improve performance of parallel initial reads using getAll(). The only exception is if there is a mix of document IDs and queries (cannot be batched together in one request). Regardless, currently all reads are queued after the documents:beginTransaction call anyways so there is no downside.
• If there are no reads before the transaction is committed, then documents:commit is called without a transaction.

Thoughts?

[brettwillis]

@tom-andersen I'm pinging you here because of your comment #1994 (comment) quoted below.

There is an unfortunate implementation detail that transactions will send a begin transaction request, followed by your get document requests.

While the PR #2002 you mention may improve performance when readTime is specified, it doesn't look like it addresses the more common performance problem and proposal described here?

[tom-andersen]

@brettwillis Thank you very much for your interest in this optimization!

Everything you describe is absolutely correct. 😍

Our team has discussed this optimization previously. All we need to do is prioritize this SDK work. We rely heavily on community feedback to guide our development priorities, and you just became a data point in favor of this development.

[brettwillis]

No worries! Would you be interested in accepting a PR, or would you prefer to keep it within the team?

[tom-andersen]

For something like this that doesn't require changes to the API surface, we will very happily accept a PR!