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.

Sign up for GitHub

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

Jump to bottom

Remove pointer math (comparison) from render caching, as it is unreliable #962

Merged
merged 2 commits into from
Feb 19, 2016
Merged

Remove pointer math (comparison) from render caching, as it is unreliable #962

merged 2 commits into from
Feb 19, 2016

Conversation

paulbellamy
Copy link
Contributor

As mentioned in #896 (comment)

Might fix the flakey test? Not sure. Either way reflect.Value.Pointer of a func is unreliable.

The tradeoff is that we have individual caches for each renderer, and have to forward ResetCache calls throughout the tree, which is ugly.

@2opremio
Copy link
Contributor

The code/approach looks fine. However, I don't know enough about this part of the code to confirm whether it's complete or it's missing edge cases.

@paulbellamy
Copy link
Contributor Author

I feel the same. Will have reviewed by @tomwilkie

@paulbellamy paulbellamy assigned tomwilkie and unassigned 2opremio Feb 16, 2016
@paulbellamy
Copy link
Contributor Author

After in-person discussion fons suggests generating a stable/random ID for each renderer, and using a single-cache against that.

@paulbellamy
generate an ID for each memoiser so they can share cache
0379fc4 
Makes cache clearing easier, and then we don't need to add
`ResetCache()` to the Renderer interface.
@2opremio
Copy link
Contributor

Much cleaner 👍

That said, it won't work against mutable renderers. Are all renderers immutable?

@paulbellamy
Copy link
Contributor Author

afaik, yes.

@tomwilkie
Copy link
Contributor

Are all renderers immutable?

They are all deterministic (which I think is the property we want here).

The renderers themselves are structs which are not inherently immutable, but we don't mutate them anywhere.

@tomwilkie
Copy link
Contributor

Might fix the flakey test?

I could get this to flake in a loop pretty easily. Will test tomorrow with this fix.

tomwilkie
tomwilkie reviewed Feb 18, 2016
View reviewed changes
render/memoise.go
default:
return m.Renderer.Render(rpt)
}
key := fmt.Sprintf("%s-%s", rpt.ID, m.id)

This comment was marked as abuse.

Sign in to view

@tomwilkie
Copy link
Contributor

Flakes reliably on master:

Toms-MacBook-Pro:app twilkie$ while ./app.test --test.run TestAll; do sleep 0.1; done
PASS
PASS
PASS
PASS
--- FAIL: TestAll (0.01s)
    scope_test.go:78: api_topology_test.go:39: Expected status 200, got 404. Path: /api/topology/containers-by-hostname/container%3A5e4d3c2b1a
FAIL
Toms-MacBook-Pro:app twilkie$ while ./app.test --test.run TestAll; do sleep 0.1; done
PASS
PASS
PASS
PASS
--- FAIL: TestAll (0.02s)
    scope_test.go:78: api_topology_test.go:39: Expected status 200, got 404. Path: /api/topology/containers-by-image/container%3A5e4d3c2b1a
FAIL
Toms-MacBook-Pro:app twilkie$ while ./app.test --test.run TestAll; do sleep 0.1; done
PASS
PASS
PASS
PASS
PASS
PASS
PASS
PASS
PASS
PASS
PASS
PASS
PASS
PASS
PASS
PASS
--- FAIL: TestAll (0.02s)
    scope_test.go:78: api_topology_test.go:39: Expected status 200, got 404. Path: /api/topology/containers-by-image/container_image%3Aimageid123
FAIL

Ran for 5mins on this branch with no flakes.

tomwilkie added a commit that referenced this pull request Feb 19, 2016
@tomwilkie
Merge pull request #962 from weaveworks/no-pointer-math
0402f6d 
Remove pointer math (comparison) from render caching, as it is unreliable
@tomwilkie tomwilkie merged commit 0402f6d into master Feb 19, 2016
@tomwilkie tomwilkie deleted the no-pointer-math branch February 19, 2016 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@tomwilkie tomwilkie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@paulbellamy @2opremio @tomwilkie