refactor: http #2386

Shylock-Hg · 2024-07-09T13:06:13Z

Summary:
Create http for each thread

Issue Reference(s):
Close #2370
/claim #2370

Build & Testing:

I ran cargo test successfully.
I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

I have added relevant unit & integration tests.
I have updated the documentation accordingly.
I have performed a self-review of my code.
PR follows the naming convention of <type>(<optional scope>): <title>

tusharmath · 2024-07-09T13:10:09Z

src/cli/runtime/http.rs

@@ -182,8 +183,41 @@ impl HttpIO for NativeHttp {
        )
        .await?)
    }
+
+    fn cl(&self) -> Box<dyn HttpIO> {
+        Box::new(self.clone())


Cloning isn't going to help, ideally we want to create separate connection pools and caches for each http-client.

I have a question about this. According to https://docs.rs/reqwest/0.12.5/reqwest/struct.Client.html, reqwest::Client has a connection pool in it already. Do we need more work?

Nothing special needs to be done for pooling.

tusharmath · 2024-07-09T13:11:27Z

src/cli/runtime/http.rs

+        if let Some(http) = self.map.get(&thread_id) {
+            http.value().execute(request).await
+        } else {
+            let new_http = self.http.cl();


Should call init_http instead.

tusharmath · 2024-07-09T13:13:08Z

src/cli/runtime/http.rs

+
+pub struct ConcurrentHttp {
+    http: Box<dyn HttpIO>,
+    map: DashMap<std::thread::ThreadId, Box<dyn HttpIO>>,


instead of map we can use a thread local.

github-actions · 2024-07-09T13:27:03Z

Bencher

Report	Thu, July 11, 2024 at 03:17:03 UTC
Project	tailcall
Branch	2386/merge
Testbed	benchmarking-runner

Click to view all benchmark results

Benchmark	Latency	Latency Results nanoseconds (ns) \| (Δ%)	Latency Upper Boundary nanoseconds (ns) \| (%)
from_json_bench	✅ (view plot)	6,125,900.00 (-10.75%)	7,787,675.01 (78.66%)
group_by	✅ (view plot)	486.00 (-11.59%)	624.18 (77.86%)
input/args.missing	✅ (view plot)	25.55 (+5.61%)	27.75 (92.07%)
input/args.nested.existing	✅ (view plot)	52.24 (+10.07%)	61.13 (85.45%)
input/args.nested.missing	✅ (view plot)	37.47 (-1.50%)	40.83 (91.76%)
input/args.root	✅ (view plot)	50.42 (+15.10%)	58.09 (86.81%)
input/headers.existing	✅ (view plot)	30.39 (-3.97%)	34.04 (89.26%)
input/headers.missing	✅ (view plot)	29.28 (-4.80%)	33.91 (86.37%)
input/value.missing	✅ (view plot)	23.43 (+0.09%)	24.96 (93.87%)
input/value.nested.existing	✅ (view plot)	42.04 (+0.85%)	45.17 (93.06%)
input/value.nested.missing	✅ (view plot)	36.85 (-0.29%)	39.60 (93.08%)
input/value.root	✅ (view plot)	38.02 (+0.44%)	41.39 (91.86%)
input/vars.existing	✅ (view plot)	7.75 (+0.75%)	9.07 (85.40%)
input/vars.missing	✅ (view plot)	7.78 (-22.35%)	13.44 (57.86%)
synth_nested	✅ (view plot)	154,830.00 (+121.74%)	209,402.78 (73.94%)
test_batched_body	✅ (view plot)	1,982.10 (-87.92%)	337,319.79 (0.59%)
test_batched_body #2	✅ (view plot)	1,633,700.00 (-3.55%)	1,817,439.01 (89.89%)
test_data_loader	✅ (view plot)	387,390.00 (-14.93%)	525,843.52 (73.67%)
test_handle_request	✅ (view plot)	144,150.00 (-6.50%)	172,369.45 (83.63%)
test_handle_request_jit	✅ (view plot)	91,685.00 (-0.11%)	94,720.38 (96.80%)
test_http_execute_method	✅ (view plot)	14,023.00 (-19.52%)	20,740.88 (67.61%)
with_mustache_expressions	✅ (view plot)	1,139.10 (-1.76%)	1,236.53 (92.12%)
with_mustache_literal	✅ (view plot)	769.23 (+6.37%)	781.41 (98.44%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

codecov · 2024-07-09T13:27:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.27%. Comparing base (c89690f) to head (3460edd).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2386   +/-   ##
=======================================
  Coverage   87.26%   87.27%           
=======================================
  Files         243      243           
  Lines       22524    22531    +7     
=======================================
+ Hits        19656    19663    +7     
  Misses       2868     2868

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tusharmath · 2024-07-10T13:56:29Z

Moving to draft to reduce noise and improve CI efficiency. Once you are ready just mark it as "ready to review". Feel free to give a shoutout on the #contributors channel on Discord if you want immediate attention.

Let me know once the comments are fixed. Thanks!

Shylock-Hg · 2024-07-11T03:50:50Z

@tusharmath I think it's ok now. But the CI of benchmark went wrong.

tusharmath · 2024-07-11T04:56:09Z

@Shylock-Hg The PR isn't doing whats expected:

Benchmarks are failing, meaning the server doesn't work correctly.
There is use of unsafe code such as panic.
You are still creating a single client and then cloning it in an arch which is exactly what we don't want.

For these reasons, I am closing this PR for now. Feel free to create a new PR and ask questions on discord if the requirement isn't clear, always happy to help!

Shylock-Hg · 2024-07-11T07:28:47Z

@Shylock-Hg The PR isn't doing whats expected:

Benchmarks are failing, meaning the server doesn't work correctly.

There is use of unsafe code such as panic.

You are still creating a single client and then cloning it in an arch which is exactly what we don't want.

For these reasons, I am closing this PR for now. Feel free to create a new PR and ask questions on discord if the requirement isn't clear, always happy to help!

Ok, I misunderstand it

Shylock-Hg added 2 commits July 9, 2024 20:56

Refactor http to call thread only instance.

0b17414

Lint fix.

8c56f06

algora-pbc bot mentioned this pull request Jul 9, 2024

create a HTTP client per thread #2370

Closed

algora-pbc bot added the 🙋 Bounty claim label Jul 9, 2024

github-actions bot added the ci: benchmark Runs benchmarks label Jul 9, 2024

Shylock-Hg changed the title ~~Refact/http~~ refact: http Jul 9, 2024

Merge branch 'main' into refact/http

de62e76

tusharmath reviewed Jul 9, 2024

View reviewed changes

Shylock-Hg changed the title ~~refact: http~~ refactor: http Jul 9, 2024

tusharmath reviewed Jul 9, 2024

View reviewed changes

Shylock-Hg requested a review from tusharmath July 10, 2024 01:38

Merge branch 'main' into refact/http

1cfc19f

github-actions bot added the type: chore Routine tasks like conversions, reorganization, and maintenance work. label Jul 10, 2024

tusharmath marked this pull request as draft July 10, 2024 13:55

Shylock-Hg added 7 commits July 11, 2024 10:27

Use thread local.

a03cda3

Lint fix.

ded1ad8

Merge branch 'main' into refact/http

22934a9

Remove dashmap.

095d726

Remove dashmap.

5878e7a

Lint fix.

bed8bd5

Remove unused code.

3460edd

Shylock-Hg marked this pull request as ready for review July 11, 2024 03:50

tusharmath closed this Jul 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: http #2386

refactor: http #2386

Shylock-Hg commented Jul 9, 2024 •

edited

Loading

tusharmath Jul 9, 2024

Shylock-Hg Jul 10, 2024

tusharmath Jul 10, 2024

tusharmath Jul 9, 2024

Shylock-Hg Jul 11, 2024

tusharmath Jul 9, 2024

Shylock-Hg Jul 11, 2024

github-actions bot commented Jul 9, 2024 •

edited

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading

tusharmath commented Jul 10, 2024

Shylock-Hg commented Jul 11, 2024

tusharmath commented Jul 11, 2024

Shylock-Hg commented Jul 11, 2024

refactor: http #2386

refactor: http #2386

Conversation

Shylock-Hg commented Jul 9, 2024 • edited Loading

tusharmath Jul 9, 2024

Choose a reason for hiding this comment

Shylock-Hg Jul 10, 2024

Choose a reason for hiding this comment

tusharmath Jul 10, 2024

Choose a reason for hiding this comment

tusharmath Jul 9, 2024

Choose a reason for hiding this comment

Shylock-Hg Jul 11, 2024

Choose a reason for hiding this comment

tusharmath Jul 9, 2024

Choose a reason for hiding this comment

Shylock-Hg Jul 11, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 9, 2024 • edited Loading

Bencher

codecov bot commented Jul 9, 2024 • edited Loading

Codecov Report

tusharmath commented Jul 10, 2024

Shylock-Hg commented Jul 11, 2024

tusharmath commented Jul 11, 2024

Shylock-Hg commented Jul 11, 2024

Shylock-Hg commented Jul 9, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading

codecov bot commented Jul 9, 2024 •

edited

Loading