refactor: drop async_graphql engine from executing queries (#3223)

tailcallhq · Dec 18, 2024 · 97dde0b · 97dde0b · github-actions · Dec 18, 2024
1 parent d8947cd
commit 97dde0b
Show file tree

Hide file tree

Showing 13 changed files with 35 additions and 69 deletions.
diff --git 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
@@ -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
@@ -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<crate::core::config::ConfigModule> 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
@@ -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<bool>,
 
     #[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
@@ -119,30 +119,17 @@ async fn execute_query<T: DeserializeOwned + GraphQLRequestLike>(
     request: T,
     req: Parts,
 ) -> anyhow::Result<Response<Body>> {
-    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
@@ -64,16 +64,16 @@ impl Conditions {
     }
 }
 
-pub struct Builder {
+pub struct Builder<'a> {
     pub index: Arc<Index>,
     pub arg_id: Counter<usize>,
     pub field_id: Counter<usize>,
-    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
@@ -88,10 +88,8 @@ impl<'a, Value: Deserialize<'a> + Clone + 'a + JsonLike<'a> + std::fmt::Debug> J
 
     fn plan(query: &str, variables: &Variables<async_graphql::Value>) -> OperationPlan<Value> {
         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
@@ -75,6 +75,8 @@ impl JITExecutor {
         &self,
         request: async_graphql::Request,
     ) -> impl Future<Output = AnyResponse<Vec<u8>>> + 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<jit::Request<Value>> for async_graphql::Request {
     fn from(value: jit::Request<Value>) -> Self {
         let mut request = async_graphql::Request::new(value.query);

diff --git a/src/core/jit/request.rs b/src/core/jit/request.rs
@@ -42,7 +42,7 @@ impl Request<ConstValue> {
         blueprint: &Blueprint,
     ) -> Result<OperationPlan<async_graphql_value::Value>> {
         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
@@ -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
@@ -273,11 +273,7 @@ impl ExecutionSpec {
         env: HashMap<String, String>,
         http: Arc<Http>,
     ) -> Arc<AppContext> {
-        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
@@ -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
@@ -3,7 +3,7 @@ source: tests/core/spec.rs
 expression: formatter
 snapshot_kind: text
 ---
-schema @server(enableJIT: true) @upstream {
+schema @server @upstream {
   query: Query
 }
Thread Stats	Avg	Stdev	Max	+/- Stdev
Latency	661.21us	368.69us	19.52ms	87.90%
Req/Sec	38.20k	1.60k	43.83k	75.42%