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

Add Dolphin version and current video backend to shader compilation logs #8336

Merged
merged 1 commit into from Jan 25, 2020

Conversation

Ebola16
Copy link
Member

@Ebola16 Ebola16 commented Aug 26, 2019

Implements https://bugs.dolphin-emu.org/issues/9992

This makes it easier to quickly identify what version of Dolphin and video backend produced shader compilation issues.

Copy link
Member

@BhaaLseN BhaaLseN left a comment

Choose a reason for hiding this comment

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

I don't know if those files are meant to be loaded into a shader compiler as-is or not (and as such: whether we should make those two new lines a comment in the given language); but the changes themselves seem fine to me. Untested.

@Ebola16
Copy link
Member Author

Ebola16 commented Aug 27, 2019

I've never tried loading the output directly into a shader compiler but if that was the intended use I could add the Dolphin info to the filename instead.

@stenzek
Copy link
Contributor

stenzek commented Nov 8, 2019

While potentially useful for debugging, this will actually force drivers which use a hash of the shader code as an identifier in their cache to recompile all shaders every Dolphin build.

@Ebola16
Copy link
Member Author

Ebola16 commented Nov 8, 2019

Ok, all of these concerns should be fixed if I add this information to the filename instead. Is this better?

Source/Core/VideoBackends/D3DCommon/Shader.cpp Outdated Show resolved Hide resolved
Source/Core/VideoBackends/D3DCommon/Shader.cpp Outdated Show resolved Hide resolved
@Ebola16
Copy link
Member Author

Ebola16 commented Jan 5, 2020

@leoetlino Is this better?

@leoetlino leoetlino dismissed their stale review January 5, 2020 12:58

outdated review

@leoetlino leoetlino requested a review from stenzek January 5, 2020 12:58
@Ebola16
Copy link
Member Author

Ebola16 commented Jan 5, 2020

I can't reproduce @stenzek's concerns anymore but it's worth a check.

@Ebola16
Copy link
Member Author

Ebola16 commented Jan 23, 2020

@stenzek?

Copy link
Contributor

@stenzek stenzek left a comment

Choose a reason for hiding this comment

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

AFAICT the intent here is changed compared to the original PR, the main difference is it's adding the backend name to the filenames. I'm not sure how useful it will be, but it's a harmless change.

I suppose as long as the actual shader code itself isn't changed, you could append the version info to the log file. But doing so at the start of the file would clobber the line references in the errors, so it'd have to be at the end.

Source/Core/VideoCommon/VideoBackendBase.cpp Outdated Show resolved Hide resolved
@Ebola16 Ebola16 changed the title Add Dolphin version and current video backend to shader compilation logs Add Dolphin version and current video backend to bad shader filename Jan 24, 2020
@Ebola16
Copy link
Member Author

Ebola16 commented Jan 24, 2020

@stenzek I think I like adding Dolphin version and current video backend to just the filename more now. Since the bad shaders are likely to raise errors themselves, adding a log message would probably be redundant. This PR will help identify where bad shaders originate, which is useful if you're testing a build that generates a lot of them and then forget to clear them. I think this PR is ready for merge now!

@stenzek
Copy link
Contributor

stenzek commented Jan 24, 2020

By "log file" I was referring to the bad_nnnn.txt file. You could tack something like "Dolphin version NNN, video backend YYY" and any other relevant configuration options at the end of the file.

Putting the version in the filename has the potential to leave lots of files in the dump directory, especially if the user has a buggy driver and tries multiple Dolphin versions. At least with the current behavior, those files will be overwritten each time.

I think putting the backend name in the filename is a good idea though.

@Ebola16
Copy link
Member Author

Ebola16 commented Jan 24, 2020

My alphabetically sorted naming convention produces:
bad_vs_5_0_Dolphin [PS] 5.0-11429-dirty_D3D_0
bad_vs_5_0_Dolphin [PS] 5.0-11429-dirty_D3D_1
bad_vs_5_0_Dolphin [PS] 5.0-11429-dirty_D3D12_0
bad_vs_5_0_Dolphin [PS] 5.0-11429-dirty_D3D12_1
bad_vs_5_0_Dolphin [PS] 5.0-11429-dirty_OGL_0
bad_vs_5_0_Dolphin [PS] 5.0-11429-dirty_OGL_1

Moving the counter before the version and video backend could would split up the video backends, producing less desirable results. It also may be confused as part of the shader stage.
bad_vs_5_0_0_Dolphin [PS] 5.0-11429-dirty_D3D
bad_vs_5_0_0_Dolphin [PS] 5.0-11429-dirty_D3D12
bad_vs_5_0_0_Dolphin [PS] 5.0-11429-dirty_OGL
bad_vs_5_0_1_Dolphin [PS] 5.0-11429-dirty_D3D
bad_vs_5_0_1_Dolphin [PS] 5.0-11429-dirty_D3D12
bad_vs_5_0_1_Dolphin [PS] 5.0-11429-dirty_OGL

I think I prefer my naming convention for the reasons stated above.

Also, I was hoping to include the version since that would be immensely helpful for determining if a bad shader file is still relevant. When I encounter bad shaders (thankfully only on old builds), it's always an endless spam of them or Dolphin crashes. Does it really matter if we add to the endless spam? And since the naming system before this PR overwrote old files, it's possible to compare bad shaders from different versions without realizing it.

I suppose we could add an option to disable bad shader logs but that's probably for a separate PR.

@Ebola16
Copy link
Member Author

Ebola16 commented Jan 24, 2020

Oh wait, I just understood what you said. Give me a few moments to try something.

@stenzek
Copy link
Contributor

stenzek commented Jan 24, 2020

I'm less concerned about the ordering of the fields in the filename and more about the version itself being included. Let's say a bad driver causes 20 files to be generated, if a user tries 5 Dolphin versions thinking it's a bug on our end, they'll end up 100 dump files in the folder. If the version string is not included, there would still only be 20.

As someone who works on the shader generators, having to manually clean the folder all the time would also get annoying. With the current behavior, I know that the 0th or 1st file is always going to be the first bad shader from the current version. With the version in the filename, I'd first have to sort by the timestamp.

Hence why I suggested appending the version string and any other info at the end of the file contents - that way the information is still there if needed for a bug report, etc, but doesn't clog up the folder with additional, duplicate files.

@Ebola16 Ebola16 changed the title Add Dolphin version and current video backend to bad shader filename Add Dolphin version and current video backend to shader compilation logs Jan 24, 2020
@Ebola16
Copy link
Member Author

Ebola16 commented Jan 24, 2020

@stenzek Updated with requested changes.

Copy link
Contributor

@stenzek stenzek left a comment

Choose a reason for hiding this comment

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

LGTM

@stenzek stenzek merged commit e3a7922 into dolphin-emu:master Jan 25, 2020
@Ebola16 Ebola16 deleted the PS branch January 25, 2020 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants