mesa: 18.2.2 -> 18.2.3 #48733
Merged
mesa: 18.2.2 -> 18.2.3 #48733
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Success on aarch64-linux (full log) Attempted: mesa Partial log (click to expand)
|
|
Success on x86_64-darwin (full log) Attempted: mesa Partial log (click to expand)
|
|
Timed out, unknown build status on x86_64-linux (full log) Attempted: mesa Partial log (click to expand)
|
|
build-id sounds like it will do the trick, but if it's not getting set the disk cache will be disabled. I'll have a look through the mesa changes to see how it's supposed to be set. |
|
Am I crazy, or will this just take the first byte of the sha1? That seems like a terrible idea. I don't even really like the idea of taking the first uint32_t, but it seems like that was the intention. |
|
On Mon, 22 Oct 2018 08:05:08 -0700, David McFarland ***@***.***> wrote:
https://gitlab.freedesktop.org/mesa/mesa/commit/83ea8dd99bb16e5d9bb880e64cd2047abc536b70#6ab322842dcea8eb54ec156adebb56a70bf10ace_115_125
Am I crazy, or will this just take the first byte of the sha1? That seems like a terrible idea. I don't even really like the idea of taking the first uint32_t, but it seems like that was the intention.
Wow, good catch!
Looks like that was unintentional, agreed.
Even so, as you point out uint32_t is still worrisome...
This seems to be a pattern, I wonder what the explanation is?
The pattern I'm referring to is one I've seen elsewhere
(such as in some places of LLVM, although it's since been
changed although IIRC it was for some other reasons)
where a cache key is derived from a (to me, shockingly) small
portion of a much larger hash that's careful to be derived
from all the relevant input parameters.
It's like if we decided to delete all but the first 2 characters
of store paths and deduplicate according to what matched :(.
But honestly I've seen this in code from folks who I respect quite a bit
so I'm rather curious what the reasoning might be.
Anyone know? :)
For fun, some brainstorming guesses:
* "If correct behavior depended on lack of collisions, using more bits
from the hash will only serve to avoid the bug in testing but
your customers will find it eventually-- so might as well 'force the
issue' by using a small prefix so testing encounters this if it matters"
* concerns about performance of copying larger key around ?
* Not sure this makes sense considering you're still computing the
full width hash anyway... but maybe there's situations this makes
sense.
* Similarly: challenges abstracting over hash technique used
or length of hash, use size known to surely be smaller than
anything used on all archs now or in the future (32bits for example).
* "32bits is good enough for everyone"
* Maybe it's the default approach in domains they work in,
so it was what they reached for when implementing this.
Regardless glad I made sure to get some review attention on this before
proceeding!
…--------
Unrelated to the hashing issue, but just wanted to report that as my
"daily driver" this version works great (xps 9560)-- which says nothing
about validity of hash concerns but at least confirms otherwise it's
functional in at least one deployment :).
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#48733 (comment) part: text/html
|
|
Yeah, I guess we all have a threshold for accepting a hash identity and for me it's somewhere between crc32 and sha256. :) It looks like
RADV/ANV actually seem properly merge the hash, e.g. https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/amd/vulkan/radv_device.c#L60 That sort of implies that it's on it's way out, and just left for compatibility with the timestamp method. Given that, I probably wouldn't complain about it being 32-bit, but 8-bit is just asking for trouble. It might be worth keeping our hash key patch until this gets resolved upstream. Do you know if it has conflicts? |
|
On Mon, 22 Oct 2018 18:11:04 +0000 (UTC), David McFarland ***@***.***> wrote:
@dtzWill
Yeah, I guess we all have a threshold for accepting a hash identity and for me it's somewhere between crc32 and sha256. :)
Haha yep.
It looks like `disk_cache_get_function_identifier` is "only" used in:
- nouveau
- r600
- radeonsi
RADV/ANV actually seem properly merge the hash, e.g. https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/amd/vulkan/radv_device.c#L60
That sort of implies that it's on it's way out, and just left for compatibility with the timestamp method. Given that, I probably wouldn't complain about it being 32-bit, but 8-bit is just asking for trouble.
Okay, agreed!
It might be worth keeping our hash key patch until this gets resolved upstream. Do you know if it has conflicts?
Yeah there are conflicts, don't know how painful:
… applying patch /nix/store/x57ng5ashm6whivib0rgryqqz1k7wndz-disk_cache-include-dri-driver-path-in-cache-key.patch
patching file src/util/Makefile.am
Hunk #1 succeeded at 35 (offset 5 lines).
patching file src/util/disk_cache.c
Hunk #1 FAILED at 388.
Hunk #2 FAILED at 410.
2 out of 2 hunks FAILED -- saving rejects to file src/util/disk_cache.c.rej
builder for '/nix/store/nnkr5q3jbdaq5jpw1bsambk3lxlj1dy7-mesa-noglu-18.2.3.drv' failed with exit code 1
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#48733 (comment) part: text/html
|
|
@dtzWill I can take a look tonight. It shouldn't be difficult, because it was just adding an extra string to the key. Just let me know if you get to it before me. |
|
On Mon, 22 Oct 2018 19:21:18 +0000 (UTC), David McFarland ***@***.***> wrote:
@dtzWill I can take a look tonight. It shouldn't be difficult, because it was just adding an extra string to the key. Just let me know if you get to it before me.
Good to hear, and nope it's all yours (sorry and thank you!).
Of course please let me/us know if it's more difficult than anticipated :).
…
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#48733 (comment) part: text/html
|
|
Failing in one in 2^32 cases wouldn't seem too worrisome, given how quickly the IDs might realistically change. |
|
@vcunat Yeah, agreed. Even an 8-bit collision on successive releases is unlikely, but it's just likely enough that I don't want anyone to ever have to find that problem the hard way. :) I pushed a fixup in case you want to squash it into your PR: corngood@fda9b5f The patch applies cleanly, so I don't expect any problems. I'm just building it locally now. |
|
@vcunat For future reference, it looks like https://sourceware.org/binutils/docs-2.31/ld/Options.html#Options |
https://lists.freedesktop.org/archives/mesa-dev/2018-October/207343.html Optimistically drop disk cache patch, changelog mentions related changes to cache behavior. Not sure if those changes address our problem (can we count on build-id?) so someone more familiar with this should probably take a look before merging :).
dtzWill
force-pushed
the
update/mesa-18.2.3
branch
from
49b9ef6
to
f7b3f43
Compare
|
Failure on x86_64-darwin (full log) Attempted: mesa Partial log (click to expand)
|
|
If this gets merged we should be able to drop the cache id patch: https://lists.freedesktop.org/archives/mesa-dev/2018-October/207606.html |
|
Success on aarch64-linux (full log) Attempted: mesa Partial log (click to expand)
|
|
Success on x86_64-linux (full log) Attempted: mesa Partial log (click to expand)
|
|
@GrahamcOfBorg mesa |
|
On Mon, 29 Oct 2018 09:55:58 -0700, David McFarland ***@***.***> wrote:
@dtzWill @vcunat This looks good to me. The darwin failure is unrelated. Any concerns with merging this?
Sounds good to me!
…
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#48733 (comment) part: text/html
|
vcunat
added a commit
that referenced
this pull request
Oct 29, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://lists.freedesktop.org/archives/mesa-dev/2018-October/207343.html
Optimistically drop disk cache patch, changelog mentions related
changes to cache behavior.
Not sure if those changes address our problem
(can we count on build-id?) so someone more familiar with
this should probably take a look before merging :).
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)