Avoid long heavy tasks in the GraphQL service #2340

xgreenx · 2024-10-13T16:27:52Z

This PR adds chunking to the stream in GraphQL and requests data in batches.

The current implementation is simple and executes batches in the same runtime. But at the end of batch fetching, it yields, allowing other tasks to be processed.

Checklist

New behavior is reflected in tests

Before requesting review

I have reviewed the code myself

…Iterator`

crates/fuel-core/src/graphql_api/database.rs

MitchTurner · 2024-10-13T17:11:08Z

crates/fuel-core/src/graphql_api/database.rs

@@ -134,7 +141,7 @@ impl ReadView {
    pub fn transaction(&self, tx_id: &TxId) -> StorageResult<Transaction> {
        let result = self.on_chain.transaction(tx_id);
        if result.is_not_found() {
-            if let Some(tx) = self.old_transaction(tx_id)? {
+            if let Some(tx) = self.off_chain.old_transaction(tx_id)? {


I prefer the old helper... although the meaning of "old" here isn't clear to me.

crates/fuel-core/src/graphql_api/database.rs

rymnc · 2024-10-14T06:24:31Z

bin/fuel-core/src/cli/run/graphql.rs

@@ -12,6 +12,10 @@ pub struct GraphQLArgs {
    #[clap(long = "port", default_value = "4000", env)]
    pub port: u16,

+    /// The size of the batch fetched from the database by GraphQL service.
+    #[clap(long = "graphql-database-batch-size", default_value = "100", env)]
+    pub database_batch_size: usize,


does this have to be configurable? can we just set it to a global now to reduce the config surface? we have too many options rn :)

I decided to make it configurable for now to be able to adjust our performance on the fly. Later we can remove it when we have better ideas about the best value

…ng-heavy-tasks

Co-authored-by: Mårten Blankfors <marten@blankfors.se>

…re/avoid-long-heavy-tasks # Conflicts: # CHANGELOG.md # crates/fuel-core/src/query/balance/asset_query.rs # crates/fuel-core/src/query/coin.rs # crates/fuel-core/src/query/message.rs # crates/fuel-core/src/query/tx.rs

netrome

A bunch of nits, but otherwise looks good to me.

crates/fuel-core/src/database/block.rs

crates/fuel-core/src/graphql_api/database.rs

netrome · 2024-10-14T08:15:33Z

crates/fuel-core/src/query/balance/asset_query.rs

                })
+            });
+
+        futures::stream::StreamExt::chunks(stream, database.batch_size)


We could just refer to self.database here

Suggested change

futures::stream::StreamExt::chunks(stream, database.batch_size)

futures::stream::StreamExt::chunks(stream, self.database.batch_size)

Or add a getter for batch_size :)

netrome · 2024-10-14T08:16:14Z

crates/fuel-core/src/query/balance/asset_query.rs

            })
+            .try_filter_map(move |chunk| async move {
+                let chunk = database


And here

Suggested change

let chunk = database

let chunk = self.database

crates/fuel-core/src/query/coin.rs

crates/fuel-core/src/query/block.rs

crates/fuel-core/src/query/message.rs

…re/avoid-long-heavy-tasks # Conflicts: # CHANGELOG.md # bin/fuel-core/src/cli/run.rs # bin/fuel-core/src/cli/run/graphql.rs # crates/fuel-core/src/graphql_api.rs # crates/fuel-core/src/service/config.rs

…ng-heavy-tasks

MitchTurner

I have one outstanding comment, but that's mostly a nit. Otherwise LGTM. Will wait until there is another review to approve.

The base branch was changed.

# Conflicts: # CHANGELOG.md # crates/fuel-core/src/coins_query.rs # crates/fuel-core/src/graphql_api/database.rs # crates/fuel-core/src/query/balance.rs # crates/fuel-core/src/query/balance/asset_query.rs # crates/fuel-core/src/query/block.rs # crates/fuel-core/src/query/coin.rs # crates/fuel-core/src/query/message.rs # crates/fuel-core/src/query/tx.rs # crates/fuel-core/src/schema/block.rs # crates/fuel-core/src/schema/tx.rs

crates/fuel-core/src/schema/tx.rs

netrome

Nice stuff! Just one question about fusing the underlying stream in YieldStream.

crates/services/src/yield_stream.rs

Voxelot · 2024-10-14T21:32:48Z

crates/fuel-core/src/graphql_api/database.rs

+            .map(|tx_id| self.transaction(tx_id))
+            .collect::<Vec<_>>();
+        // Give a chance to other tasks to run.
+        tokio::task::yield_now().await;


what is the point of this yield? This task has already finished using the database by now.

Should we be applying the same chunk batching to the iterator above?

The idea is that in the future, we will have an async caching mechanism where we will wait for notification when the cache fetches a new desired value(which potentially can be used by several queries in the last block with transactions).

This yield_now imitates this behavior, also allowing Tokio to work with other tasks.

Voxelot · 2024-10-14T21:48:32Z

crates/fuel-core/src/query/balance/asset_query.rs

                })
+            });
+
+        futures::stream::StreamExt::chunks(stream, database.batch_size)


could this all be cleaned up with the new yield_each extension to avoid the need to map chunks and flatten results?

No, here we actually send a batch request to the database like: "Please fetch the all coins for these list of UtxoIds".

Later we can optimize it with multi get + caching(next follow up PR will add caching)

Oic, since it might be multiget later?

crates/fuel-core/src/query/coin.rs

crates/fuel-core/src/query/message.rs

crates/fuel-core/src/query/tx.rs

@rymnc

## Version v0.40.0 ### Added - [2347](#2347): Add GraphQL complexity histogram to metrics. - [2350](#2350): Added a new CLI flag `graphql-number-of-threads` to limit the number of threads used by the GraphQL service. The default value is `2`, `0` enables the old behavior. - [2335](#2335): Added CLI arguments for configuring GraphQL query costs. ### Fixed - [2345](#2345): In PoA increase priority of block creation timer trigger compare to txpool event management ### Changed - [2334](#2334): Prepare the GraphQL service for the switching to `async` methods. - [2310](#2310): New metrics: "The gas prices used in a block" (`importer_gas_price_for_block`), "The total gas used in a block" (`importer_gas_per_block`), "The total fee (gwei) paid by transactions in a block" (`importer_fee_per_block_gwei`), "The total number of transactions in a block" (`importer_transactions_per_block`), P2P metrics for swarm and protocol. - [2340](#2340): Avoid long heavy tasks in the GraphQL service by splitting work into batches. - [2341](#2341): Updated all pagination queries to work with the async stream instead of the sync iterator. - [2350](#2350): Limited the number of threads used by the GraphQL service. #### Breaking - [2310](#2310): The `metrics` command-line parameter has been replaced with `disable-metrics`. Metrics are now enabled by default, with the option to disable them entirely or on a per-module basis. - [2341](#2341): The maximum number of processed coins from the `coins_to_spend` query is limited to `max_inputs`. ## What's Changed * fix(gas_price_service): service name and unused trait impl by @rymnc in #2317 * Do not require build of docker images to pass CI by @xgreenx in #2342 * Prepare the GraphQL service for the switching to `async` methods by @xgreenx in #2334 * Limited the number of threads used by the GraphQL service by @xgreenx in #2350 * Increase priority of timer over txpool event by @xgreenx in #2345 * Disable flaky `test_poa_multiple_producers` test by @rafal-ch in #2353 * feat: CLI arguments for configuring GraphQL query costs. by @netrome in #2335 * Add graphql query complexity histogram metric by @AurelienFT in #2349 * Updated all pagination queries to work with the `Stream` instead of `Iterator` by @xgreenx in #2341 * Avoid long heavy tasks in the GraphQL service by @xgreenx in #2340 * Add more metrics by @rafal-ch in #2310 **Full Changelog**: v0.39.0...v0.40.0 --------- Co-authored-by: Rafał Chabowski <rafal.chabowski@fuel.sh> Co-authored-by: acerone85 <andrea.cerone@gmail.com> Co-authored-by: rymnc <43716372+rymnc@users.noreply.github.com> Co-authored-by: Rafał Chabowski <88321181+rafal-ch@users.noreply.github.com>

xgreenx added 7 commits October 11, 2024 19:31

Prepare the GraphQL service for the switching to async methods

2fe7bb6

Updated CHANGELOG.md

ab5e940

Updated all pagination queries to work with the Stream instead of `…

1782684

…Iterator`

Making change non-breaking for now

5192c95

Updated CHANGELOG.md

feeb816

Avoid long heavy tasks in the GraphQL service

4237feb

Use correct naming

142cd1d

xgreenx requested a review from a team October 13, 2024 16:27

xgreenx self-assigned this Oct 13, 2024

xgreenx requested review from Dentosal and MitchTurner as code owners October 13, 2024 16:27

xgreenx added 2 commits October 13, 2024 18:28

Updated CHANGELOG.md

671e80e

Make closure more readable

2354d10

MitchTurner reviewed Oct 13, 2024

View reviewed changes

rymnc reviewed Oct 14, 2024

View reviewed changes

Base automatically changed from feature/async-pagination-queries to feature/prepare-graphql-for-async October 14, 2024 06:37

xgreenx force-pushed the feature/prepare-graphql-for-async branch from 7cb4231 to ab5e940 Compare October 14, 2024 06:48

xgreenx changed the base branch from feature/prepare-graphql-for-async to feature/async-pagination-queries October 14, 2024 06:51

xgreenx and others added 3 commits October 14, 2024 08:59

Apply comment from parent PR

d982fbc

Use more clear naming for the logic

c355cd1

Merge branch 'feature/async-pagination-queries' into feature/avoid-lo…

7846db4

…ng-heavy-tasks

xgreenx requested review from rymnc and MitchTurner October 14, 2024 07:05

Fix flakiness

5a752bc

netrome mentioned this pull request Oct 14, 2024

Updated all pagination queries to work with the Stream instead of Iterator #2341

Merged

2 tasks

xgreenx and others added 4 commits October 14, 2024 10:09

Apply suggestions from code review

a8c1942

Co-authored-by: Mårten Blankfors <marten@blankfors.se>

Apply comments from PR

df80801

Merge branch 'master' into feature/prepare-graphql-for-async

e414046

netrome previously approved these changes Oct 14, 2024

View reviewed changes

xgreenx and others added 11 commits October 14, 2024 12:13

Make CI happy

5e0ec92

Merge branch 'master' into feature/async-pagination-queries

981b690

Merge with master

3521a72

Merge branch 'refs/heads/feature/async-pagination-queries' into featu…

946f46c

…re/avoid-long-heavy-tasks # Conflicts: # CHANGELOG.md # bin/fuel-core/src/cli/run.rs # bin/fuel-core/src/cli/run/graphql.rs # crates/fuel-core/src/graphql_api.rs # crates/fuel-core/src/service/config.rs

Merge branch 'master' into feature/async-pagination-queries

c4ad916

Merge branch 'feature/async-pagination-queries' into feature/avoid-lo…

2866da1

…ng-heavy-tasks

Merge branch 'master' into feature/async-pagination-queries

d8e159c

Merge branch 'feature/async-pagination-queries' into feature/avoid-lo…

c61436e

…ng-heavy-tasks

Merge branch 'master' into feature/async-pagination-queries

71d3692

Merge branch 'master' into feature/async-pagination-queries

35c7184

Merge branch 'feature/async-pagination-queries' into feature/avoid-lo…

d8c8c5e

…ng-heavy-tasks

MitchTurner reviewed Oct 14, 2024

View reviewed changes

Base automatically changed from feature/async-pagination-queries to master October 14, 2024 19:53

xgreenx requested review from MitchTurner and netrome October 14, 2024 20:04

AurelienFT approved these changes Oct 14, 2024

View reviewed changes

crates/fuel-core/src/schema/tx.rs Show resolved Hide resolved

rymnc approved these changes Oct 14, 2024

View reviewed changes

netrome approved these changes Oct 14, 2024

View reviewed changes

crates/services/src/yield_stream.rs Show resolved Hide resolved

Voxelot reviewed Oct 14, 2024

View reviewed changes

crates/fuel-core/src/query/coin.rs Show resolved Hide resolved

Voxelot reviewed Oct 14, 2024

View reviewed changes

crates/fuel-core/src/query/message.rs Show resolved Hide resolved

Voxelot reviewed Oct 14, 2024

View reviewed changes

crates/fuel-core/src/query/tx.rs Show resolved Hide resolved

Voxelot approved these changes Oct 14, 2024

View reviewed changes

xgreenx merged commit 87c9579 into master Oct 14, 2024
38 checks passed

xgreenx deleted the feature/avoid-long-heavy-tasks branch October 14, 2024 21:58

xgreenx mentioned this pull request Oct 14, 2024

Release v0.40.0 #2356

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid long heavy tasks in the GraphQL service #2340

Avoid long heavy tasks in the GraphQL service #2340

xgreenx commented Oct 13, 2024

MitchTurner Oct 13, 2024

Voxelot Oct 14, 2024

rymnc Oct 14, 2024

xgreenx Oct 14, 2024

netrome left a comment

netrome Oct 14, 2024

MitchTurner Oct 14, 2024

netrome Oct 14, 2024

MitchTurner left a comment

netrome left a comment

Voxelot Oct 14, 2024 •

edited

Loading

xgreenx Oct 14, 2024 •

edited

Loading

Voxelot Oct 14, 2024 •

edited

Loading

xgreenx Oct 14, 2024 •

edited

Loading

Voxelot Oct 14, 2024

xgreenx Oct 14, 2024

	futures::stream::StreamExt::chunks(stream, database.batch_size)
	futures::stream::StreamExt::chunks(stream, self.database.batch_size)

Avoid long heavy tasks in the GraphQL service #2340

Avoid long heavy tasks in the GraphQL service #2340

Conversation

xgreenx commented Oct 13, 2024

Checklist

Before requesting review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MitchTurner left a comment

Choose a reason for hiding this comment

netrome left a comment

Choose a reason for hiding this comment

Voxelot Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

xgreenx Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Voxelot Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

xgreenx Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Voxelot Oct 14, 2024 •

edited

Loading

xgreenx Oct 14, 2024 •

edited

Loading

Voxelot Oct 14, 2024 •

edited

Loading

xgreenx Oct 14, 2024 •

edited

Loading