Refactor: Reduce File Browser Sorting #119103

Harley Acheson · 2024-03-06T00:37:38+01:00

Harley Acheson commented

2024-03-06 00:37:38 +01:00

Eliminate multiple worst-case scenario file list sorting.

This sounds improbable, so this needs some explanation.

On my Windows computer, when we ask for a directory file listing using API call FindNextFileW the items come out in alphabetical order (for NTFS and CDFS). So the list is A-Z

But when we are using that function to assemble the list - in bli_builddir - we add each item using addhead so the list becomes alphabetically backwards . Z-A

Once that is done we call qsort - also in bli_builddir - on the list to make it (naturally) alphabetically forward (and also put folders first). Because of the previous step we have made this sort almost as difficult as possible. A-Z

Now that we have this alphabetical list we then load that into file browser lists, but we do so by going through the list in reverse order - in filelist_readjob_list_dir. Z-A

After this the file listings are then sorted again as the user wishes. And if this is forward alpha order then we are again making this sort as difficult as possible. But now finally A-Z

So A-Z -> Z-A -> A-Z -> Z-A -> A-Z

This PR keeps the items in order before sorting in bli_builddir, then processes the items in order in filelist_readjob_list_dir, eliminating the reversals.

Note that I have reversed the processing order inside filelist_readjob_list_dir instead of merely changing a single addtail to an addhead. This is because there could (later) be utility in going through the list in order, especially since it is sorted. For example if we ever want to collapse image sequences.

Eliminate multiple worst-case scenario file list sorting. --- This sounds improbable, so this needs some explanation. On my Windows computer, when we ask for a directory file listing using API call `FindNextFileW ` the items come out in alphabetical order (for NTFS and CDFS). So the list is A-Z But when we are using that function to assemble the list - in `bli_builddir` - we add each item using addhead so the list becomes alphabetically backwards . Z-A Once that is done we call qsort - also in `bli_builddir` - on the list to make it (naturally) alphabetically forward (and also put folders first). Because of the previous step we have made this sort almost as difficult as possible. A-Z Now that we have this alphabetical list we then load that into file browser lists, but we do so by going through the list in **reverse order** - in `filelist_readjob_list_dir`. Z-A After this the file listings are then sorted again as the user wishes. And if this is forward alpha order then we are again making this sort as difficult as possible. But now finally A-Z **So A-Z -> Z-A -> A-Z -> Z-A -> A-Z** This PR keeps the items in order before sorting in bli_builddir, then processes the items in order in filelist_readjob_list_dir, eliminating the reversals. Note that I have reversed the processing order inside `filelist_readjob_list_dir` instead of merely changing a single addtail to an addhead. This is because there could (later) be utility in going through the list in order, especially since it is sorted. For example if we ever want to collapse image sequences.

Harley Acheson added 1 commit 2024-03-06 00:37:49 +01:00

276e8b6f89 Refactor: Reduce File Browser Sorting

Eliminate multiple worst-case scenario file list sorting.

Harley Acheson added this to the User Interface project 2024-03-06 01:03:34 +01:00

Harley Acheson requested review from Hans Goudey 2024-03-06 01:18:23 +01:00

Hans Goudey reviewed 2024-03-06 17:03:25 +01:00

Hans Goudey left a comment

First of all, nice find! Improving this does seem nice. And if there's sorting going on after every step, changing the order of the lists in intermediate cases should matter.

Two main thoughts though:

What is the motivating goal of the PR? If it's code cleanup, that's nice, but apart from replacing the old while (i--) style for loop, there isn't much improvement. If it's performance, it would be good to include some performance numbers in a large directory or something.
The PR description has much more information than the final commit message here. I think there could be a balance, so someone reading the commit in a few years would at least have some understanding of why "addhead" was changed to "addtail"

First of all, nice find! Improving this does seem nice. And if there's sorting going on after every step, changing the order of the lists in intermediate cases should matter. Two main thoughts though: 1. What is the motivating goal of the PR? If it's code cleanup, that's nice, but apart from replacing the old `while (i--)` style for loop, there isn't much improvement. If it's performance, it would be good to include some performance numbers in a large directory or something. 2. The PR description has _much_ more information than the final commit message here. I think there could be a balance, so someone reading the commit in a few years would at least have some understanding of why "addhead" was changed to "addtail"

source/blender/editors/space_file/filelist.cc Outdated

						
				@ -3141,0 +3136,4 @@

				    entries_num++;

				  }

				  if (files) {

Hans Goudey commented

2024-03-06 16:48:49 +01:00

It looks like moving the if (files) check is an unrelated cleanup? The diff would be clearer without that.

It looks like moving the `if (files)` check is an unrelated cleanup? The diff would be clearer without that.

Harley marked this conversation as resolved

Harley Acheson added 1 commit 2024-03-06 18:26:27 +01:00

ce144c8814 Merge branch 'main' of projects.blender.org:blender/blender into FileSorting

Harley Acheson added 1 commit 2024-03-06 18:28:27 +01:00

951583a7f0 Minimum changes to stop the lists from reversing.

Harley Acheson commented

2024-03-06 18:40:30 +01:00

@HooglyBoogly

All good points. I updated this PR so that it is the minimum changes to stop the list reversals.

There really isn't anything here that would be a measurable performance change in regular usage. I was only amused while looking around in there.

My only motivation was in taking some notes regarding a right-click select suggestion and wanted to remind myself at where would would need sorting. At a place I expected an unsorted order I saw a reversed alpha natural sort so followed the flow from the OS on through.

@HooglyBoogly All good points. I updated this PR so that it is the _minimum_ changes to stop the list reversals. There really isn't anything here that would be a measurable performance change in regular usage. I was only amused while looking around in there. My only motivation was in taking some notes regarding a [right-click select suggestion](https://blender.community/c/rightclickselect/LJvv/) and wanted to remind myself at where would would need sorting. At a place I expected an unsorted order I saw a reversed alpha natural sort so followed the flow from the OS on through.

Hans Goudey commented

2024-03-06 19:29:19 +01:00

There really isn't anything here that would be a measurable performance change in regular usage. I was only amused while looking around in there.

Okay, maybe this is a party-pooper stance, but if there is no performance gain and no improvement in code quality, I don't get the point of this PR. I think generally each commit should have a clear motivation-- as it stands this one doesn't really.

>There really isn't anything here that would be a measurable performance change in regular usage. I was only amused while looking around in there. Okay, maybe this is a party-pooper stance, but if there is no performance gain and no improvement in code quality, I don't get the point of this PR. I think generally each commit should have a clear motivation-- as it stands this one doesn't really.

Harley Acheson commented

2024-03-06 20:41:22 +01:00

No worries.

Harley Acheson closed this pull request

2024-03-06 20:41:25 +01:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Refactor: Reduce File Browser Sorting #119103

Pull request closed