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

Feature/disable caching #410

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Feature/disable caching #410

wants to merge 3 commits into from

Conversation

ikelos
Copy link
Member

@ikelos ikelos commented Dec 30, 2020

This feature adds support for (very early) deciding as to whether caching of layer reads should be enabled throughout the codebase. It's not ideal because the main toggle doesn't live in volatility.frameworks.constants like it should, because volatility.framework imports other parts of the framework that already need to know whether the caching is enabled or not. To avoid this issue, the toggle lives just in volatility.

@ikelos ikelos requested a review from npetroni December 30, 2020 18:01
@ikelos ikelos self-assigned this Dec 30, 2020
@@ -22,6 +22,11 @@
from typing import Dict, Type, Union, Any
from urllib import parse, request

import volatility

if "--live" in sys.argv:
Copy link

Choose a reason for hiding this comment

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

Hmm, this doesn't seem right. Why isn't this handled by the argparse logic below the --live option?

Copy link
Member Author

@ikelos ikelos Jan 6, 2021

Choose a reason for hiding this comment

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

So I concur, it's thoroughly ugly, but unfortunately the chain of imports (of volatility.framework upwards) would import and therefore wrap all the calls before we've set the flag. So we've got two options:

  • for every read check whether we're allowed to use the cache, and then use it or not
  • wrap everything early with a wrapper that chooses whether to cache or not

From an efficiency perspective, the second option is better (I haven't timed to see how much, and chances are it's a couple lookups and an if, so should be pretty fast, but given our runs regularly do hundreds of thousands of reads, I went with the slight ugliness in the CLI to set the flag early enough. I might look into a way so that change the variable late will throw an exception (so that UI developers know they need to set it really early, I'm just not sure how I'd go about making a variable read-only after a certain point...

Anyway, that's why it needs to be read out of sys.argv so early. Once the parser's run, we've already imported everything to be able to enumerate it properly.

@npetroni
Copy link

npetroni commented Jan 6, 2021

What if we tried something more localized to caching, like this?

https://github.com/volatilityfoundation/volatility3/tree/feature/disable-caching-try2

@ikelos
Copy link
Member Author

ikelos commented Jan 6, 2021

That won't trip in time (and you can add an exception in to show that changing the flag would have no effect). The wrapping and therefore that if statement are evaluated as the files are imported. By the time you've reached there, they're all wrapped already. You'd need to write your own lru_cache method which would check whether to use the cache or not each time if you wanted to be changable dynamically.

@ikelos
Copy link
Member Author

ikelos commented Jan 6, 2021

Another option may be to do through and explicitly unwrap all wrapped objects (or maybe set their cache size to 0?), but that would be monkeying deep inside of python and probably make it difficult to distinguish if there were some values we wanted cached and others we didn't...

@npetroni
Copy link

npetroni commented Jan 6, 2021

We can discuss it today. The exception trips for me, so I am missing something.

@ikelos
Copy link
Member Author

ikelos commented Jan 6, 2021

Yeah, no problem. Adding the exception in after the if statement in disable-caching-try2, I get the exception whether I'm using --live or not, meaning the flag isn't getting interpreted before the wrapper's being called (and the caching is always being applied), but I'm happy to go through it at the meeting. 5:)

@ikelos
Copy link
Member Author

ikelos commented Jan 6, 2021

Following a discussion, we've considered that performance (particularly for the common case) is important, but this approach is a bit complex/ugly so we're going to defer making this change for a bit.

@ikelos ikelos changed the base branch from master to develop January 6, 2021 21:20
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.

2 participants