Closed NelsonVides closed 2 years ago
Merging #183 (41fd7c0) into main (dcae5ca) will decrease coverage by
0.06%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## main #183 +/- ##
==========================================
- Coverage 92.85% 92.79% -0.07%
==========================================
Files 10 10
Lines 448 430 -18
==========================================
- Hits 416 399 -17
+ Misses 32 31 -1
Impacted Files | Coverage Δ | |
---|---|---|
src/wpool_sup.erl | 100.00% <ø> (ø) |
|
src/wpool_pool.erl | 96.35% <100.00%> (+0.27%) |
:arrow_up: |
src/wpool_queue_manager.erl | 88.46% <100.00%> (-0.15%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update dcae5ca...41fd7c0. Read the comment docs.
I don't understand what CodeCov is complaining about… So, I'll just ignore that thing and merge anyway…
I don't understand what CodeCov is complaining about… So, I'll just ignore that thing and merge anyway…
I analyzed the situation manually and I figured out the issue is just that the module has now fewer lines of code. 🤦🏻♂️
Code is quite opinionated, and we'd need to take very serious the fact that this would make dynamically adding or removing workers to a pool very very hard, as it would require modifying a persistent_term record.
It's not terrible and that is a slightly utopic feature anyway…
Couldn't resist the temptation 😱
TL;DR;
The PT implementation spends A LOT less time finding the pool details. At some point perfs seems to complain we now spend more time in getting the process info than anything else (grabbing the main_lock for the process and seeing the message queue length and so on, can't get any faster than that by now).
How it is all done: the wpool record contains now one more field with all the worker names as a tuple, so that not only the names are pre-generated, but also getting them is a O(1)
element(N #wpool.workers)
op. Also,#wpool.next
is now anatomics_ref()
, so that the rotation for the next worker doesn't take any pt modifications – the idea is from https://www.erlang.org/blog/persistent_term/ and already successfully applied by me in https://github.com/esl/segmented_cache 😄Then all strategies just get the
#wpool
record from the persistent_term storage, and operate on fully shared and immutable data, so many other helper functions are removed in favour of justfind_wpool(Name)
.Code is quite opinionated, and we'd need to take very serious the fact that this would make dynamically adding or removing workers to a pool very very hard, as it would require modifying a persistent_term record.
Time for opinions 😄
reports
fprof
Note a general improvement of 25% over finding the worker and applying all the strategy, and also, some 25% less garbage collections.
In main:
in the persistent_term branch:
perf
Note how there's way more time now on timer:tc or in io:format, proportionally.
main
persistent_term