Optimisations and cleanups #173
Conversation
NelsonVides
force-pushed
the
optimisations_and_cleanups
branch
from
b29b665 to
4041d2b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
🚀 New features to boost your workflow:
|
NelsonVides
force-pushed
the
optimisations_and_cleanups
branch
2 times, most recently
from
90a3ec7 to
32492a9
Compare
NelsonVides
requested review from
deadtrickster,
mikpe,
mkuratczyk,
onno-vos-dev,
DenysGonchar,
MirahImage and
lhoguin
src/metrics/prometheus_counter.erl
Outdated
| @@ -90,7 +90,7 @@ inc(Caller) -> | |||
| -define(TABLE, ?PROMETHEUS_COUNTER_TABLE). | |||
| -define(ISUM_POS, 2). | |||
| -define(FSUM_POS, 3). | |||
| -define(WIDTH, 16). | |||
| -define(WIDTH, erlang:system_info(schedulers_online)). | |||
There was a problem hiding this comment.
Maybe instead of fixing this to be schedulers_online we can set this up as an application:get_env(<application>, <some_name>, 16) so the original remains preserved but we open up the loophole for setting a higher value.
There was a problem hiding this comment.
Another issue is that schedulers_online can change value dynamically at runtime.
There was a problem hiding this comment.
Right. Should use just schedulers, as that can't change at runtime.
On the other hand I'm not sure it'd help to make it configurable, currently the algorithm that chooses the key would do X band (?WIDTH - 1), so most values would mean skewing the distribution of schedulers to each key and some keys would see more contention than others. Sensibly, if we're keying on the scheduler id, the only values that make sense are multiples (or halves) of the number of schedulers.
src/metrics/prometheus_histogram.erl
Outdated
| @@ -95,7 +95,7 @@ the number of time ticks when the recent value was observed). | |||
| -define(ISUM_POS, 3). | |||
| -define(FSUM_POS, 4). | |||
| -define(BUCKETS_START, 5). | |||
| -define(WIDTH, 16). | |||
| -define(WIDTH, erlang:system_info(schedulers_online)). | |||
There was a problem hiding this comment.
Maybe instead of fixing this to be schedulers_online we can set this up as an application:get_env(<application>, <some_name>, 16) so the original remains preserved but we open up the loophole for setting a higher value.
NelsonVides
force-pushed
the
optimisations_and_cleanups
branch
from
32492a9 to
ebd885f
Compare
NelsonVides
force-pushed
the
optimisations_and_cleanups
branch
from
ebd885f to
eb19b61
Compare
Note that the histogram table actually requires read concurrency, as on every insert first parameters need to be looked up. Otherwise this makes it more flexible to configure any table independently and precisely, may the need arise.
Note that the current configuration would distribute the load badly: the key is chosen by bitwise-and on the scheduler ID, but if we have more schedulers than 16, then some keys would see twice as much contention than others, see for example, if we had 24: ``` 1> [ X band 15 || X <- lists:seq(1, 24)]. [1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,0,1,2,3,4,5,6,7,8] ```
Examine the condition inlined in the helper instead of passing a function object. Also ensure that the entry point has checked the types, when looping on the inner function, the JIT compiler will know already that the inputs are lists and numbers, skipping dynamic type checking on every iteration. The `lists` module implements such optimisation extensively.
NelsonVides
force-pushed
the
optimisations_and_cleanups
branch
from
eb19b61 to
e2c2b50
Compare
This PR proposes a miscellaneous of updates and improvements to the repo. Commits are well organised by topics and with good descriptions, so that'd be the best way to review the whole thing, as some changes are not related to each other – but are some times too small to be moved to their own PR, which I could easily do if any of you would prefer me to do anyways.
EDIT: #173 (comment)