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

eliminate unused memoisation #2923

Closed
rade opened this issue Nov 4, 2017 · 4 comments · Fixed by #2924
Closed

eliminate unused memoisation #2923

rade opened this issue Nov 4, 2017 · 4 comments · Fixed by #2924
Assignees
Labels
chore Related to fix/refinement/improvement of end user or new/existing developer functionality performance Excessive resource usage and latency; usually a bug or chore

Comments

@rade
Copy link
Member

rade commented Nov 4, 2017

Rendering makes use of memoisation, by wrapping filters, maps, reducers and conditionalRenderers in render.Memoise structs. The objective here is to

  • evaluate common sub-expressions of a rendering function only once
  • evaluate the top-level rendering functions only once when they are invoked multiple times on the same report (e.g. when the browser requests the same rendering for the same timespan and no new reports have been received for that timespan)

The memoisation is accomplished in the renderer constructors, i.e. MakeFilter, MakeFilterPseudo, MakeMap, MakeReduce, ConditionalRenderer, by wrapping with a render.Memoise struct.

That actually leads to a lot of unnecessary memoisation, since many of these constructors are in places that are neither common sub-expressions nor the top-level.

We should remove the memoisation from the constructors and instead insert it at the call sites that are in these places.

@rade rade added chore Related to fix/refinement/improvement of end user or new/existing developer functionality performance Excessive resource usage and latency; usually a bug or chore labels Nov 4, 2017
@bboreham
Copy link
Collaborator

bboreham commented Nov 4, 2017

Are there good examples when memoisation is expected to help common subexpressions?

@rade
Copy link
Member Author

rade commented Nov 4, 2017

It definitely happens - a while ago I instrumented the memoisation code and it showed plenty of cache hits even in a single render of a report.

@rade rade self-assigned this Nov 5, 2017
@bboreham
Copy link
Collaborator

bboreham commented Nov 5, 2017

Here's one:

HostRenderer (called by the hosts api) calls ColorConnectedProcessRenderer, and calls ContainerRenderer which also calls ColorConnectedProcessRenderer.

Ironically ColorConnectedProcessRenderer is not memoised.

@rade
Copy link
Member Author

rade commented Nov 5, 2017

I fixed that in the #2924 :)

@rade rade closed this as completed in #2924 Nov 6, 2017
rade added a commit that referenced this issue Nov 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Related to fix/refinement/improvement of end user or new/existing developer functionality performance Excessive resource usage and latency; usually a bug or chore
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants