Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove unused memoisation #2924

Merged
merged 5 commits into from
Nov 6, 2017
Merged

remove unused memoisation #2924

merged 5 commits into from
Nov 6, 2017

Conversation

rade
Copy link
Member

@rade rade commented Nov 5, 2017

Remove all memoisation and then re-introduce it a) for top-level renderes, i.e. those closest to the API, and b) shared renderes.

This cuts the number of memoising renderers from 74 to 21.

I ran a number of tests with a recent prod report

prog/scope --mode=app --weave=false --app.collector=file:report.json

and then timed the fetching of topologies with

time curl -so /dev/null http://localhost:4040/api/topology/<topology>
topology befor rpeat ahost hits after rpeat ahost hits
processes 0.315 0.108 0.125 0/1/1 0.312 0.108 0.124 0/1/1
processes-by-name 0.249 0.029 0/1 0.229 0.026 0/1
containers 1.282 0.399 0.421 0/1/1 1.276 0.416 0.432 0/1/1
containers-by-hostname 0.926 0.053 1/1 0.906 0.058 1/1
containers-by-image 0.924 0.034 0/1 0.898 0.038 0/1
pods 0.954 0.074 0.082 1/1/1 0.960 0.080 0.089 2/1/1
kube-controllers 0.947 0.042 1/1 0.943 0.039 1/1
services 0.960 0.031 1/1 0.936 0.031 1/1
hosts 1.278 0.025 4/1 1.237 0.025 4 /1

rpeat = fetching the same topology again
ahost = fetching the topology after first fetching the host topology
hits = memoisation cache hits during fetch / rpeat fetch / ahost fetch

Timings are best run out of five.

So there really is no difference in timing, but it should consume less memory.

Each of the memoisations does have an effect, often very substantial; i.e. disabling any one increases at least one of the numbers above.

Fixes #2923

rade added 4 commits November 4, 2017 22:21
we will re-introduce it more selectively later
This deals with browsers requesting the same rendering for the same
timespan when no new reports have been received for that timespan.
and add a comment indicating non-memoisation of other, not shared
top-level renderers.

This memoisation is effective when the browser requests multiple
topologies for the same report.
@rade rade requested a review from bboreham November 5, 2017 01:25
We do this here rather than in ConnectionJoin since that way it is
obvious that the renderer isn't memoised already.
@rade rade merged commit b3669be into master Nov 6, 2017
@dlespiau dlespiau deleted the 2923-memoise-less branch November 21, 2017 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eliminate unused memoisation
2 participants