Caching: factorizing / toughening

[valschneider]

I've been struggling to reproduce the bugs generated by caching (I did encounter them in the past so I'm not doubting their existence), as such I mostly went through the code and improved what I thought needed to be improved.

I'm not re-enabling caching in this PR, but I will try to convince some people to re-enable it on their side and do some bug-finding for me.

[valschneider]

• Removed noise
• Squashed window test commit into window-aware commit

[valschneider]

I'm looking into using a Tempfile instead of a trace.txt whenever possible. Sadly, I realised my "Delete trace.txt if caching is enabled" commit isn't actually working. If the txt file is correctly deleted after the cache is created, it is re-generated even if the cache exists. I can fix this but it requires a fair amount of rework, stay tuned.

[valschneider]

GenericFTrace has gone through a facelift, and I've broken systrace (I haven't tested it but I'm sure I did), however it looks like there are no systrace tests so I'll add some next week.

This "v2" doesn't have any window check as @bjackman suggested, but it has:

• Shiny new factorized code for GenericFtrace
• trace-cmd report outputted to a tempfile that is deleted once parsing is complete

[bjackman]

Cool, thanks, I'll start testing this first and then review later.

[valschneider]

Surprisingly, I hadn't broken systrace. I did make it parse the file twice though, this is fixed now. I also realized that removing the file extension from the cache dir name wasn't such a smart idea: there could be a trace.dat and trace.html living in the same directory, and they should each get their own cache.

tl;dr:

• Fixed Systrace and added Systrace tests
• Reverted cache directory format from ".trace.cache/" to ".trace.{txt/dat/html}.cache/"

[valschneider]

@bjackman: I had a chat with @derkling, we could have a fairly simple handling of the windows. If caching is enabled, we would disregard windows and parse the whole trace to cache it, and then return the requested window. As such, whatever window may be requested later on, no more trace parsing would be required.

[bjackman]

OK, makes sense

[valschneider]

Pushed that cached-window-handling thing, but for some reason the PR page won't update.

[valschneider]

Okay looks like it eventually made its way there.

[valschneider]

@bjackman I think I addressed all of yours comments.

I also made a slight change to _do_parse: I factorized the windowification and time normalization in a _apply_user_parameters method

[valschneider]

Food for thoughts: had a chat with @derkling, following the "disregard window" initiative we could also parse all possible events to store them in the cache, so we would only need to cache a trace once, no matter the parameters. This might be a bit more extreme than to simply disregard windows, so it might be better to have it in another PR - but it's a thing to consider, at least.

[bjackman]

Sorry, that "Close and comment" button always looks so much like a "cancel posting this comment".

Food for thoughts: had a chat with @derkling, following the "disregard window" initiative we could also parse all possible events to store them in the cache, so we would only need to cache a trace once, no matter the parameters. This might be a bit more extreme than to simply disregard windows, so it might be better to have it in another PR - but it's a thing to consider, at least.

Basically: yeah, agree.

[valschneider]

Can't seem to reply directly to 2 of your comments, replying here:

Huh, so an event is dropped if it's earlier than either window[0]s or later than both window[1]s? That doesn't sound good. But OK, let's leave it for another day.

We never encounter this because we either use no window, or just one (never both at the same time). But yes, it's a bit weird.

Ahhh I see. Can we have a comment like "now that we know the basetime we can derive max_window"?

Will do.

[valschneider]

Removed the window checks altogether from _do_parse()

[valschneider]

Alright, we're getting there - thanks a lot @bjackman for the careful review.

I've (once again) re-arranged the commits and did some pedantic changes here and there, but it's roughly what it was before with the following changes:

• Fiddling with trace metadata made me realise what I called 'cache_info' is really just metadata, so the file is now named 'metadata.json' and it makes more sense to me.
• Extra data can be appended in to this file with by overriding the _get_extra_data_to_cache and _load_extra_data_from_cache methods. In the case of FTrace, this is used to store trace metadata (lines of data that appear in the trace but are not actual trace events).
• Added cached/uncached FTrace metadata test

I think it would be clearer to put self.__populate_metadata at the end of init directly, putting it here gives a false impression that it needs to be done at this specific stage of parsing.

Right now it has to be in there, otherwise metadata will not be cached. Might not be the most obvious implementation though.

[valschneider]

Fixed an annoying warning that said cache was invalid when it simply hadn't been created yet.

[valschneider]

• s/_max_window()/_calc_max_window()/g
• Fixed the merge conflict with #271

[valschneider]

I think I've addressed all of your comments:

• Fixed the metadata caching test
• Turned the caching metadata methods into proper OOP citizens

[valschneider]

• Fixed merge conflict with #272

[valschneider]

Undesired whitespaces have been trialled and sentenced.