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

feat: detect more perf data file types in directory #677

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

lievenhey
Copy link
Contributor

Currently hotspot dir only opens perf.data files. This patch extends this to perf.data, perf.data.old and perf.data.perfparser files.

@GitMensch
Copy link
Contributor

From the commit and PR title I've thought this to be about "open all perf.data/.perfparser files in the given directory"...

Not sure about perf.old.data... what do you think about moving it at least after perf.data.perfparser or take both out of that check and list dir directory for any files starting with "perf.data" or ending on ".perfparser"?

@lievenhey lievenhey changed the title feat: open more perf files feat: detect more perf data file types in directory Sep 17, 2024
@lievenhey lievenhey requested a review from milianw September 17, 2024 16:57
@lievenhey
Copy link
Contributor Author

right, I changed the title so it is less misleading.

Not sure about perf.old.data... what do you think about moving it at least after perf.data.perfparser or take both out of that check and list dir directory for any files starting with "perf.data" or ending on ".perfparser"?

The intend of this feature is to emulate perf report. Therefor it doesn't make sense to open all files, only the newest one. But yes it should be not the second in the list. Lets see what @milianw has to say.

@GitMensch
Copy link
Contributor

Opening the newest one would make it unlikely to load perf.old.data - but yes, I'd find it very nice if specifying the directory just takes the newest file per our file open- re

hotspot/src/mainwindow.cpp

Lines 287 to 294 in e6ce4c4

QString MainWindow::queryOpenDataFile()
{
const auto filter = tr("Hotspot data Files (perf*.data perf.data.* *.perfparser *.perfparser.*);;"
"Linux Perf Files (perf*.data perf.data.*);;"
"Perfparser Files (*.perfparser *.perfparser.*);;"
"All Files (*)");
return QFileDialog::getOpenFileName(this, tr("Open File"), QDir::currentPath(), filter);
}

Those are more files and currently there's no list dir, order by timestamp - can you please inspect adding this in this PR?

src/main.cpp Outdated Show resolved Hide resolved
@milianw
Copy link
Member

milianw commented Sep 26, 2024

I agree with Simon, remove the .old and extract that into a single function that allows us to share that code

@lievenhey
Copy link
Contributor Author

done

Currently `hotspot dir` only opens perf.data files. This patch extends
this to perf.data, perf.data.old and perf.data.perfparser files.
@lievenhey
Copy link
Contributor Author

I just noticed, that this should not be merged, until #678 is merged. That PR makes opening compressed files much easier.
I should add compressed files to findPerfDataFiles

Copy link
Contributor

@GitMensch GitMensch left a comment

Choose a reason for hiding this comment

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

Note: the commit summary still says "perf.data.old" (I still suggest to actually do as the function name findPerfDataFiles - not search for a list of 2 fixed entries but to search for perf*.data perf.data.* *.perfparser *.perfparser.*, ordered by date (newest first).

if (QFile::exists(perfDataFile) && window) {
window->openFile(perfDataFile);
const auto perfDataFiles = findPerfDataFiles();
for (const auto& perfDataFile : perfDataFiles) {
Copy link
Contributor

Choose a reason for hiding this comment

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

perf report opens only a single file and I have lots of perf files in my directories...
I suggest to add either an optional integer limit or boolean newestFileOnly attribute to that function and use it here and above.

Copy link
Member

Choose a reason for hiding this comment

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

the loop breaks after the first opened file, so this is fine as-is

Copy link
Contributor

Choose a reason for hiding this comment

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

In theory this means that the caller may collect lots of file entries, just to drop all of those later.
That's not of a big issue currently as findPerfDataFiles() does not match our file open window (which I think it should, I'm creating a new issue to track it).

if (QFile::exists(perfDataFile) && window) {
window->openFile(perfDataFile);
const auto perfDataFiles = findPerfDataFiles();
for (const auto& perfDataFile : perfDataFiles) {
Copy link
Member

Choose a reason for hiding this comment

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

the loop breaks after the first opened file, so this is fine as-is

@milianw milianw merged commit b0d370f into master Oct 7, 2024
25 checks passed
@milianw milianw deleted the open-perf-files branch October 7, 2024 07:55
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.

3 participants