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

Reduce memoization in targeted refresh #112

Merged
merged 1 commit into from
Jul 23, 2018

Conversation

miha-plesko
Copy link
Contributor

@miha-plesko miha-plesko commented Jul 23, 2018

We're doing a lot of unneccessary caching in targeted collector. Not anymore - with this commit we simplify the code and spare memory. Rspecs are left intact and there are no errors about missing http interactions which confirms that caching was never used - because each entity is fetched and parsed only once anyway.

RFE BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1574940

We're doing a lot of unneccessary caching in targeted collector.
Not anymore - with this commit we simplify the code and spare memory.
Rspecs are left intact and there are no errors about missing http interactions
which confirms that caching was never used - because each entity is
fetched and parsed only once anyway.

Signed-off-by: Miha Pleško <[email protected]>
Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 awesome

@Ladas Ladas merged commit 9b13050 into ManageIQ:master Jul 23, 2018
@Ladas Ladas added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 23, 2018
@miha-plesko miha-plesko deleted the reduce-memoization branch July 24, 2018 08:47
@bronaghs
Copy link

@miha-plesko Any idea what the memory savings are?

@miha-plesko
Copy link
Contributor Author

@bronaghs haven't measured memory change but IMAO most notable thing here is that code became more readable. Memory change is not too big I think because targeted refresh works with small set of entities anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants