From 97dde0bf4676f582db4435a16818fc128bfa07d5 Mon Sep 17 00:00:00 2001 From: Kiryl Mialeshka <8974488+meskill@users.noreply.github.com> Date: Wed, 18 Dec 2024 21:46:36 +0100 Subject: [PATCH] refactor: drop async_graphql engine from executing queries (#3223) --- benches/handle_request_bench.rs | 7 ++-- generated/.tailcallrc.schema.json | 6 ---- src/core/blueprint/server.rs | 2 -- src/core/config/directives/server.rs | 11 ++---- src/core/http/request_handler.rs | 35 ++++++------------- src/core/jit/builder.rs | 20 +++++------ src/core/jit/fixtures/jp.rs | 6 ++-- src/core/jit/graphql_executor.rs | 3 ++ src/core/jit/request.rs | 2 +- src/core/jit/synth/synth.rs | 2 +- tests/core/parse.rs | 6 +--- .../snapshots/test-enable-jit.md_merged.snap | 2 +- .../test-required-fields.md_merged.snap | 2 +- 13 files changed, 35 insertions(+), 69 deletions(-) diff --git a/benches/handle_request_bench.rs b/benches/handle_request_bench.rs index f33c52a895..88035b056e 100644 --- a/benches/handle_request_bench.rs +++ b/benches/handle_request_bench.rs @@ -16,13 +16,11 @@ pub fn benchmark_handle_request(c: &mut Criterion) { let sdl = std::fs::read_to_string("./ci-benchmark/benchmark.graphql").unwrap(); let config_module: ConfigModule = Config::from_sdl(sdl.as_str()).to_result().unwrap().into(); - let mut blueprint = Blueprint::try_from(&config_module).unwrap(); - let mut blueprint_clone = blueprint.clone(); + let blueprint = Blueprint::try_from(&config_module).unwrap(); let endpoints = config_module.extensions().endpoint_set.clone(); let endpoints_clone = endpoints.clone(); - blueprint.server.enable_jit = false; let server_config = tokio_runtime .block_on(ServerConfig::new(blueprint.clone(), endpoints.clone())) .unwrap(); @@ -47,9 +45,8 @@ pub fn benchmark_handle_request(c: &mut Criterion) { }) }); - blueprint_clone.server.enable_jit = true; let server_config = tokio_runtime - .block_on(ServerConfig::new(blueprint_clone, endpoints_clone)) + .block_on(ServerConfig::new(blueprint, endpoints_clone)) .unwrap(); let server_config = Arc::new(server_config); diff --git a/generated/.tailcallrc.schema.json b/generated/.tailcallrc.schema.json index 55fbf80df1..70ca3e9258 100644 --- a/generated/.tailcallrc.schema.json +++ b/generated/.tailcallrc.schema.json @@ -476,12 +476,6 @@ "null" ] }, - "enableJIT": { - "type": [ - "boolean", - "null" - ] - }, "globalResponseTimeout": { "description": "`globalResponseTimeout` sets the maximum query duration before termination, acting as a safeguard against long-running queries.", "type": [ diff --git a/src/core/blueprint/server.rs b/src/core/blueprint/server.rs index 75d5b3e186..e856154dbc 100644 --- a/src/core/blueprint/server.rs +++ b/src/core/blueprint/server.rs @@ -14,7 +14,6 @@ use crate::core::config::{self, ConfigModule, HttpVersion, PrivateKey, Routes}; #[derive(Clone, Debug, Setters)] pub struct Server { - pub enable_jit: bool, pub enable_apollo_tracing: bool, pub enable_cache_control_header: bool, pub enable_set_cookie_header: bool, @@ -124,7 +123,6 @@ impl TryFrom for Server { )) .map( |(hostname, http, response_headers, script, experimental_headers, cors)| Server { - enable_jit: (config_server).enable_jit(), enable_apollo_tracing: (config_server).enable_apollo_tracing(), enable_cache_control_header: (config_server).enable_cache_control(), enable_set_cookie_header: (config_server).enable_set_cookies(), diff --git a/src/core/config/directives/server.rs b/src/core/config/directives/server.rs index ec006e2023..b9e1897769 100644 --- a/src/core/config/directives/server.rs +++ b/src/core/config/directives/server.rs @@ -29,10 +29,9 @@ use crate::core::macros::MergeRight; /// comprehensive set of server configurations. It dictates how the server /// behaves and helps tune tailcall for various use-cases. pub struct Server { - // The `enableJIT` option activates Just-In-Time (JIT) compilation. When set to true, it - // optimizes execution of each incoming request independently, resulting in significantly - // better performance in most cases, it's enabled by default. - #[serde(default, skip_serializing_if = "is_default", rename = "enableJIT")] + #[deprecated(note = "No longer used, TODO: drop it")] + #[serde(default, skip_serializing, rename = "enableJIT")] + #[schemars(skip)] pub enable_jit: Option, #[serde(default, skip_serializing_if = "is_default")] @@ -262,10 +261,6 @@ impl Server { self.pipeline_flush.unwrap_or(true) } - pub fn enable_jit(&self) -> bool { - self.enable_jit.unwrap_or(true) - } - pub fn get_routes(&self) -> Routes { self.routes.clone().unwrap_or_default() } diff --git a/src/core/http/request_handler.rs b/src/core/http/request_handler.rs index 712929ee0d..e7ab5efae2 100644 --- a/src/core/http/request_handler.rs +++ b/src/core/http/request_handler.rs @@ -119,30 +119,17 @@ async fn execute_query( request: T, req: Parts, ) -> anyhow::Result> { - let mut response = if app_ctx.blueprint.server.enable_jit { - let operation_id = request.operation_id(&req.headers); - let exec = JITExecutor::new(app_ctx.clone(), req_ctx.clone(), operation_id); - request - .execute_with_jit(exec) - .await - .set_cache_control( - app_ctx.blueprint.server.enable_cache_control_header, - req_ctx.get_min_max_age().unwrap_or(0), - req_ctx.is_cache_public().unwrap_or(true), - ) - .into_response()? - } else { - request - .data(req_ctx.clone()) - .execute(&app_ctx.schema) - .await - .set_cache_control( - app_ctx.blueprint.server.enable_cache_control_header, - req_ctx.get_min_max_age().unwrap_or(0), - req_ctx.is_cache_public().unwrap_or(true), - ) - .into_response()? - }; + let operation_id = request.operation_id(&req.headers); + let exec = JITExecutor::new(app_ctx.clone(), req_ctx.clone(), operation_id); + let mut response = request + .execute_with_jit(exec) + .await + .set_cache_control( + app_ctx.blueprint.server.enable_cache_control_header, + req_ctx.get_min_max_age().unwrap_or(0), + req_ctx.is_cache_public().unwrap_or(true), + ) + .into_response()?; update_response_headers(&mut response, req_ctx, app_ctx); Ok(response) diff --git a/src/core/jit/builder.rs b/src/core/jit/builder.rs index 26ab6d1f66..04f99c45cb 100644 --- a/src/core/jit/builder.rs +++ b/src/core/jit/builder.rs @@ -64,16 +64,16 @@ impl Conditions { } } -pub struct Builder { +pub struct Builder<'a> { pub index: Arc, pub arg_id: Counter, pub field_id: Counter, - pub document: ExecutableDocument, + pub document: &'a ExecutableDocument, } // TODO: make generic over Value (Input) type -impl Builder { - pub fn new(blueprint: &Blueprint, document: ExecutableDocument) -> Self { +impl<'a> Builder<'a> { + pub fn new(blueprint: &Blueprint, document: &'a ExecutableDocument) -> Self { let index = Arc::new(blueprint.index()); Self { document, @@ -372,7 +372,7 @@ mod tests { let config = Config::from_sdl(CONFIG).to_result().unwrap(); let blueprint = Blueprint::try_from(&config.into()).unwrap(); let document = async_graphql::parser::parse_query(query).unwrap(); - Builder::new(&blueprint, document).build(None).unwrap() + Builder::new(&blueprint, &document).build(None).unwrap() } #[tokio::test] @@ -640,25 +640,23 @@ mod tests { let config = Config::from_sdl(CONFIG).to_result().unwrap(); let blueprint = Blueprint::try_from(&config.into()).unwrap(); let document = async_graphql::parser::parse_query(query).unwrap(); - let error = Builder::new(&blueprint, document.clone()) - .build(None) - .unwrap_err(); + let error = Builder::new(&blueprint, &document).build(None).unwrap_err(); assert_eq!(error, BuildError::OperationNameRequired); - let error = Builder::new(&blueprint, document.clone()) + let error = Builder::new(&blueprint, &document) .build(Some("unknown")) .unwrap_err(); assert_eq!(error, BuildError::OperationNotFound("unknown".to_string())); - let plan = Builder::new(&blueprint, document.clone()) + let plan = Builder::new(&blueprint, &document) .build(Some("GetPosts")) .unwrap(); assert!(plan.is_query()); insta::assert_debug_snapshot!(plan.selection); - let plan = Builder::new(&blueprint, document.clone()) + let plan = Builder::new(&blueprint, &document) .build(Some("CreateNewPost")) .unwrap(); assert!(!plan.is_query()); diff --git a/src/core/jit/fixtures/jp.rs b/src/core/jit/fixtures/jp.rs index 2d0c99b45c..f743fdc281 100644 --- a/src/core/jit/fixtures/jp.rs +++ b/src/core/jit/fixtures/jp.rs @@ -88,10 +88,8 @@ impl<'a, Value: Deserialize<'a> + Clone + 'a + JsonLike<'a> + std::fmt::Debug> J fn plan(query: &str, variables: &Variables) -> OperationPlan { let config = ConfigModule::from(Config::from_sdl(Self::CONFIG).to_result().unwrap()); - let builder = Builder::new( - &Blueprint::try_from(&config).unwrap(), - async_graphql::parser::parse_query(query).unwrap(), - ); + let doc = async_graphql::parser::parse_query(query).unwrap(); + let builder = Builder::new(&Blueprint::try_from(&config).unwrap(), &doc); let plan = builder.build(None).unwrap(); let plan = transform::Skip::new(variables) diff --git a/src/core/jit/graphql_executor.rs b/src/core/jit/graphql_executor.rs index 31afb8af0f..e1c131c374 100644 --- a/src/core/jit/graphql_executor.rs +++ b/src/core/jit/graphql_executor.rs @@ -75,6 +75,8 @@ impl JITExecutor { &self, request: async_graphql::Request, ) -> impl Future>> + Send + '_ { + // TODO: hash considering only the query itself ignoring specified operation and + // variables that could differ for the same query let hash = Self::req_hash(&request); async move { @@ -135,6 +137,7 @@ impl JITExecutor { } } +// TODO: used only for introspection, simplify somehow? impl From> for async_graphql::Request { fn from(value: jit::Request) -> Self { let mut request = async_graphql::Request::new(value.query); diff --git a/src/core/jit/request.rs b/src/core/jit/request.rs index cbd3bc0faf..f37c4a721e 100644 --- a/src/core/jit/request.rs +++ b/src/core/jit/request.rs @@ -42,7 +42,7 @@ impl Request { blueprint: &Blueprint, ) -> Result> { let doc = async_graphql::parser::parse_query(&self.query)?; - let builder = Builder::new(blueprint, doc); + let builder = Builder::new(blueprint, &doc); let plan = builder.build(self.operation_name.as_deref())?; transform::CheckConst::new() diff --git a/src/core/jit/synth/synth.rs b/src/core/jit/synth/synth.rs index e24b6c50a0..221fa95da2 100644 --- a/src/core/jit/synth/synth.rs +++ b/src/core/jit/synth/synth.rs @@ -345,7 +345,7 @@ mod tests { let config = Config::from_sdl(CONFIG).to_result().unwrap(); let config = ConfigModule::from(config); - let builder = Builder::new(&Blueprint::try_from(&config).unwrap(), doc); + let builder = Builder::new(&Blueprint::try_from(&config).unwrap(), &doc); let plan = builder.build(None).unwrap(); let plan = plan .try_map(|v| { diff --git a/tests/core/parse.rs b/tests/core/parse.rs index a438a31e89..6246c40008 100644 --- a/tests/core/parse.rs +++ b/tests/core/parse.rs @@ -273,11 +273,7 @@ impl ExecutionSpec { env: HashMap, http: Arc, ) -> Arc { - let mut blueprint = Blueprint::try_from(config).unwrap(); - - if cfg!(feature = "force_jit") { - blueprint.server.enable_jit = true; - } + let blueprint = Blueprint::try_from(config).unwrap(); let script = blueprint.server.script.clone(); diff --git a/tests/core/snapshots/test-enable-jit.md_merged.snap b/tests/core/snapshots/test-enable-jit.md_merged.snap index 3d61f7941a..99a8a80fcb 100644 --- a/tests/core/snapshots/test-enable-jit.md_merged.snap +++ b/tests/core/snapshots/test-enable-jit.md_merged.snap @@ -3,7 +3,7 @@ source: tests/core/spec.rs expression: formatter snapshot_kind: text --- -schema @server(enableJIT: true, hostname: "0.0.0.0", port: 8000) @upstream { +schema @server(hostname: "0.0.0.0", port: 8000) @upstream { query: Query } diff --git a/tests/core/snapshots/test-required-fields.md_merged.snap b/tests/core/snapshots/test-required-fields.md_merged.snap index 32c7a45a63..5cb6c7f4f3 100644 --- a/tests/core/snapshots/test-required-fields.md_merged.snap +++ b/tests/core/snapshots/test-required-fields.md_merged.snap @@ -3,7 +3,7 @@ source: tests/core/spec.rs expression: formatter snapshot_kind: text --- -schema @server(enableJIT: true) @upstream { +schema @server @upstream { query: Query }