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

Remove the "ancient PC" optimization #209

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Conversation

Borewit
Copy link
Owner

@Borewit Borewit commented Sep 25, 2023

Removes the ["ancient PC optimization"] and replaces it with a much short lambda expression.

@Borewit Borewit added debt Technical debt low-priority labels Sep 25, 2023
@Borewit Borewit self-assigned this Sep 25, 2023
@Borewit
Copy link
Owner Author

Borewit commented Sep 25, 2023

I am just curious @touwys, this PR removes the "ancient PC optimization", are you able repeat the same test?

Reviewing this PR has no priority at all.

Expected result: it may take a little longer to find the best matches.

@Borewit Borewit force-pushed the remove-ancient-pc-optimization branch from ba2736d to 6affbe8 Compare September 25, 2023 17:55
Repository owner deleted a comment from github-actions bot Sep 25, 2023
@github-actions
Copy link

@touwys
Copy link

touwys commented Sep 25, 2023

I am just curious @touwys, this PR removes the "ancient PC optimization", are you able repeat the same test?

Yes, I am. Can take care of it on Tuesday, 26th.

@touwys
Copy link

touwys commented Sep 26, 2023

@Borewit :

listFix()-2.8.1-5

The ancient pc speed test could not be brought to a successful conclusion, due to an issue that repeatedly broke the repair process. Namely, the repair process stopped immediately after having first completed finding the exact matches. Finding the exact matches was therefore not naturally followed by the next part of the process, i.e. finding the closest matches. There were no other visible clues to this occurrence, but the stop.

Nevertheless, the test was completed using the identical setup as before, and inasmuch as it concerns testing the speed of finding only the exact matches.

Setup

  1. Windows 7 x64 PC with a 4-core Intel Core i7-3770K CPU.
  2. Two different processing speed tests were run with the same playlist.
  3. The playlist contains 631 fully broken mp3 tracks calling for matches.
  4. The tests were timed.
  5. For the first test, the Media Directory (MD) panel incorporated both a mp3 and FLAC library.
  6. For the second test, the FLAC directory was removed from the MD.

Results

Test 1

Find Exact Find Closest Total
1m 7s issue-stopped

Test 2

Find Exact Find Closest Total
0m 29s issue-stopped

Comments & Questions

  1. Clearly, these identical results confirm those drawn earlier with listFix()-2.8.1.2.

  2. Support for Support for Enable/Disable Media Directories #191 can also be fruitfully applied during test-runs, and not only during repair ops. Being able to switch media directories on and off, see extremely useful application.

  3. Presumably, the search algorithm itself, has not yet been touched?


@Borewit
Copy link
Owner Author

Borewit commented Sep 26, 2023

due to an issue that repeatedly broke the repair process. Namely, the repair process stopped immediately after having first completed finding the exact matches.

So sorry, made a mistake in the cancellation logic. I took it out, as it not necessary check with within processing a single playlist entry, per playlist entry is good enough.

I overwrote the last commit, hence you get again a build with the same version number.

@Borewit Borewit force-pushed the remove-ancient-pc-optimization branch from 6affbe8 to b10ea22 Compare September 26, 2023 17:19
@github-actions
Copy link

@touwys
Copy link

touwys commented Sep 26, 2023

Build run (#6316151411, attempt #1) artifacts

Do you want me to repeat the ancient of speed test with build? Anything else perhaps, that I should check for? Can do it Wed. 27th.

@Borewit
Copy link
Owner Author

Borewit commented Sep 26, 2023

Build run (#6316151411, attempt #1) artifacts

Do you want me to repeat the ancient of speed test with build? Anything else perhaps, that I should check for?

I would be interesting to know the difference in performance between the previous optimization and this one.
As that is the effective change of this PR.

The build reflect the state before this PR can be found here: https://github.com/Borewit/listFix/actions/runs/6302944637

Please note that in the log file you can find the time it took to process the resolve the closest matches, e.g.: Resolved closest matches in 38 ms.

But it way more important that I did not break anything!

@touwys
Copy link

touwys commented Sep 26, 2023

I can do this tomorrow, no problem at all. Is it alright if I go back and forth between the two builds, when installing the one over the other all the time?

@touwys
Copy link

touwys commented Sep 26, 2023

Please note that in the log file you can find the time it took to process the resolve the closest matches, e.g.: Resolved closest matches in 38 ms.

Thanks, and there I was using a stopwatch all this time. 😆

@Borewit
Copy link
Owner Author

Borewit commented Sep 26, 2023

Thanks, and there I was using an stopwatch all this time. 😆

I admire your commitment!

@touwys
Copy link

touwys commented Sep 26, 2023

I admire your commitment!

It's mutual. 👍🏻

@touwys
Copy link

touwys commented Sep 27, 2023

Please note that in the log file you can find the time it took to process the resolve the closest matches [...]

@Borewit : I'm unable to locate those "logfile.log" files again... did the name, or location, perhaps change at all? Not desperately searching for it, though, as I'm able to carry on as before without it.

@touwys
Copy link

touwys commented Sep 27, 2023

Build run (#6316151411, attempt #1) artifacts

@Borewit : There appears to be an issue with the package installer for this build. When the installation is executed, there is a momentary flash of a process-dialogue across the screen, but the installation nothing carry on to complete:


9-27-2023.09-07-49.mp4

@Borewit
Copy link
Owner Author

Borewit commented Sep 27, 2023

I'm unable to locate those "logfile.log" files again... did the name, or location, perhaps change at all?

No change, should be in: %USERPROFILE%\.listFix()\logs, e.g. C:\Users\touwys\.listFix()\logs

I will add a section to the README to cover that.

@Borewit
Copy link
Owner Author

Borewit commented Sep 27, 2023

There appears to be an issue with the package installer for this build. When the installation is executed, there is a momentary flash of a process-dialogue across the screen, but the installation nothing carry on to complete:

I don't think it is the build. It works for me, and we are building the deployment in the pipeline which gives a very consistent result. Try to uninstall, restart computer. On the quality of the installer itself I have very little influence.

Update: but I did have similar issue testing a different deployment with the same version number. Let me investigate if that can be improved.

@touwys
Copy link

touwys commented Sep 27, 2023

Try to uninstall, restart computer. On the quality of the installer itself I have very little influence.

Thanks, I was on the verge of doing just that, when a scheduled power blackout hit.

Update: but I did have similar issue testing a different deployment with the same version number. Let me investigate if that can be improved.

Will try again tomorrow after receiving the results of your investigation.

@Borewit
Copy link
Owner Author

Borewit commented Sep 27, 2023

For me uninstalling ListFix() was enough.

I also just merged #210 which an update of a component which has some influence on the build process.

@touwys
Copy link

touwys commented Sep 27, 2023

No change, should be in: %USERPROFILE%\.listFix()\logs, e.g. C:\Users\touwys\.listFix()\logs

Thanks, I was peeking in the wrong directory i.e. %appdata%. However, using the stopwatch is fun!

@touwys
Copy link

touwys commented Sep 27, 2023

For me uninstalling ListFix() was enough.

I will do that next when I'm at the PC then, and not wait for the outcome of your checking over of the build/installer. I was not too surprised at the trouble during the installation, because I've observed in the past that Windows does not like the crisscross installation of older and newer app versions.

@touwys
Copy link

touwys commented Sep 28, 2023

@Borewit

I will do that next when I'm at the PC then, and not wait for the outcome of your checking over of the build/installer. I was not too surprised at the trouble during the installation, because I've observed in the past that Windows does not like the crisscross installation of older and newer app versions.

2023.09.28

This installation failed, too. Same symptoms. The following measures were taken before attempting the installation this time:

  1. Downloaded the build anew, at: Remove the "ancient PC" optimization · Borewit/listFix@b10ea22.

  2. Removed all traces of the previous test-build, namely that of listFix-2.8.1-4, from the Windows 7 PC.

  3. Rebooted PC.

  4. Executed listFix()-2.8.1-5 package installer.


@touwys
Copy link

touwys commented Oct 4, 2023

@Borewit

Build run (#6384221479, attempt #1) artifacts

Not sure whether I was supposed to download this build listFix()-2.8.1.8, but I did, and these are the results:

  1. The Windows-installer for this build worked just fine.

Results

Test 1: listFix()-2.8.1-4

Media Find Exact Find Closest Total
mp3+flac 1m 8s 3m 44s 4m 52s
mp3 29s 1m 58s 2m 28s

Test 2: listFix()-2.8.1-8

Media Find Exact Find Closest Total
mp3+flac 1m 9s 3m 43s 4m 52s
mp3 30s 1m 58s 2m 28s

Comments

  1. Results were virtually identical for these two builds.

  2. Stopwatch used. The given figures are rounded to next highest second.


Additional Information:

  1. As far as the json-files are concerned — when they were opened after the test run:

1.1 "history" — content:

image

1.2 "medialibrary" — not seen, notepad hung.

1.3 "options" — content

image

2. Another strange issue is observable while re-adding a directory to the Media Directories pane:

Steps:

2.1 Add a mp3 and afterwards a flac directory to the Media Directories pane.
2.2 Remove the mp3 directory.
2.3 Now, add the mp3 directory back, and, repeatedly, the flac directory gets displayed instead of flac, in the progress pop-up. (Screenshot below.)

image


@Borewit
Copy link
Owner Author

Borewit commented Oct 4, 2023

Thanks for test @touwys, seems that we can safely merge this simplified code.

I opened #215 to follow up the deprecated playlistDirectory option.

@Borewit Borewit closed this Oct 4, 2023
@Borewit Borewit reopened this Oct 4, 2023
@Borewit Borewit merged commit da15e81 into main Oct 4, 2023
6 checks passed
@github-actions
Copy link

github-actions bot commented Oct 4, 2023

@Borewit Borewit added the internal Excluded from release notes label Oct 6, 2023
@Borewit Borewit deleted the remove-ancient-pc-optimization branch October 23, 2023 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Technical debt internal Excluded from release notes low-priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants