Re: [Mesa-dev] [PATCH 0/9][RFC] GLSL preprocessor/parser improvements
2017-05-22 20:02 GMT+02:00 Thomas Helland : > 2017-05-22 3:41 GMT+02:00 Timothy Arceri : >> I suspect you forgot to test with a debug build, there is issues with the >> string buffer changes somewhere. >> >> ralloc.c:203: reralloc_size: Assertion `ralloc_parent(ptr) == ctx' failed. >> >> #0 0x770eb91f in raise () from /lib64/libc.so.6 >> #1 0x770ed51a in abort () from /lib64/libc.so.6 >> #2 0x770e3da7 in __assert_fail_base () from /lib64/libc.so.6 >> #3 0x770e3e52 in __assert_fail () from /lib64/libc.so.6 >> #4 0x7442c73c in reralloc_size (ctx=ctx@entry=0x7fffd40cb910, >> ptr=, size=) at ralloc.c:203 >> #5 0x744b519e in glcpp_preprocess >> (ralloc_ctx=ralloc_ctx@entry=0x7fffd40c9da0, >> shader=shader@entry=0x7fffea7cfcc8, info_log=info_log@entry=0x7fffd40ca058, >> extensions=extensions@entry=0x743ff070 >> > const*, int), glcpp_parser*, unsigned int, bool)>, >> state=state@entry=0x7fffd40c9da0, >> gl_ctx=gl_ctx@entry=0x7fffe85c7010) at glsl/glcpp/pp.c:246 >> #6 0x74402665 in _mesa_glsl_compile_shader >> (ctx=ctx@entry=0x7fffe85c7010, shader=shader@entry=0x7fffd40c8cc0, >> dump_ast=dump_ast@entry=false, dump_hir=dump_hir@entry=false, >> force_recompile=force_recompile@entry=false) >> at glsl/glsl_parser_extras.cpp:2065 >> >> > > Your suspicion is of course correct. Switching back and forth between > profiling and coding and I forgot to switch debug back on. I'll look into > this, plus addressing all of the nice feedback. Thanks for taking a look. > I've tracked down the issue and sent some updates to the list. The issue was related to trying to realloc with the wrong ralloc context in pp.c when resizing the buffer to reduce footprint. I've added a "crimp_to_fit" function that does exactly that. This is used instead of handrolling it in pp.c like was initially done. I've also worked on porting the tests to gtest. It compiles nicely, I just have to figure out how to run the gtest tests... It shouldn't be to far out though, so expect them on the list the coming week or so. Some performance numbers when running shader-db with GALLIUM_NOOP set on a nouveau gallium driver: Runtime drops from 48 to 45 seconds. 8% reduction in executed instruction, 6-7% drop in cycles. So we are seeing that stalled cycles due to cache misses are a primary limiting factor that's holding us back. In other words, nothing new under the sun. >> >> On 22/05/17 06:49, Thomas Helland wrote: >>> >>> This patch series contains some of the work done by Vladislav >>> in the beginning of March, that seems to have been forgotten. >>> >>> For reference, that series, with review comments, can be found here: >>> https://lists.freedesktop.org/archives/mesa-dev/2017-January/139892.html >>> >>> I've adressed some of the review comments, most notably redoing the >>> string buffer implementation quite a bit, and adding tests. >>> The major change is that it is now decoupled from the parser >>> and instead lives in /src/util/ as a utility for easier reuse. >>> I've also closely followed the structure of our hash table and set >>> so that it should be quite familiar for everyone. My implementation >>> is also null terminated, while Vladislav's implementation was not. >>> My reasoning behind this was that it's less fragile if someone where >>> to access the contents of the buffer directly. I've also expanded >>> with some different functions that allow us to do more stuff. >>> Obviously this could be expanded upon further if need be. >>> >>> I've included some of Vladislav's less involved patches that >>> I think we should get in as soon as possible. I've added Ian's >>> r-b on the ones that he reviewed (I hope that's OK). The one >>> patch that is missing from this series is the hand-written >>> custom parser. While this was the thing that gave the biggest >>> speed-up, unfortunately it's also a bit harder to review, >>> so I've left that as a future exercise. >>> >>> I've run it through my complete shader-db and there's no changes >>> or breakages, so that's encouraging. It amounts to about a 3% >>> reduction in runtime and executed instructions on the shader-db run. >>> It should be noted that the i965 backend is the primary bottleneck >>> when running shader-db on my computer, so the numbers don't look all >>> that impressive due to this. I'll see if I can come up with some >>> numbers using the gallium noop driver. >>> >>> I have not done a very thorough job on testing that the output >>> of the preprocessor/parser is the same after this series. >>> However, there's no changes here that I believe should cause >>> any issues, so I feel quite confident that this series should >>> not cause any trouble. The original series did have some issues >>> discovered by running a fuzzer over it, however I expect that >>> was caused by the introduced hand-written fast-path scanner and >>> associated macro substitutaion that are not a part of this series. >>> >>> CC: Vladislav Egorov
Re: [Mesa-dev] [PATCH 0/9][RFC] GLSL preprocessor/parser improvements
2017-05-22 3:41 GMT+02:00 Timothy Arceri : > I suspect you forgot to test with a debug build, there is issues with the > string buffer changes somewhere. > > ralloc.c:203: reralloc_size: Assertion `ralloc_parent(ptr) == ctx' failed. > > #0 0x770eb91f in raise () from /lib64/libc.so.6 > #1 0x770ed51a in abort () from /lib64/libc.so.6 > #2 0x770e3da7 in __assert_fail_base () from /lib64/libc.so.6 > #3 0x770e3e52 in __assert_fail () from /lib64/libc.so.6 > #4 0x7442c73c in reralloc_size (ctx=ctx@entry=0x7fffd40cb910, > ptr=, size=) at ralloc.c:203 > #5 0x744b519e in glcpp_preprocess > (ralloc_ctx=ralloc_ctx@entry=0x7fffd40c9da0, > shader=shader@entry=0x7fffea7cfcc8, info_log=info_log@entry=0x7fffd40ca058, > extensions=extensions@entry=0x743ff070 > const*, int), glcpp_parser*, unsigned int, bool)>, > state=state@entry=0x7fffd40c9da0, > gl_ctx=gl_ctx@entry=0x7fffe85c7010) at glsl/glcpp/pp.c:246 > #6 0x74402665 in _mesa_glsl_compile_shader > (ctx=ctx@entry=0x7fffe85c7010, shader=shader@entry=0x7fffd40c8cc0, > dump_ast=dump_ast@entry=false, dump_hir=dump_hir@entry=false, > force_recompile=force_recompile@entry=false) > at glsl/glsl_parser_extras.cpp:2065 > > Your suspicion is of course correct. Switching back and forth between profiling and coding and I forgot to switch debug back on. I'll look into this, plus addressing all of the nice feedback. Thanks for taking a look. > > On 22/05/17 06:49, Thomas Helland wrote: >> >> This patch series contains some of the work done by Vladislav >> in the beginning of March, that seems to have been forgotten. >> >> For reference, that series, with review comments, can be found here: >> https://lists.freedesktop.org/archives/mesa-dev/2017-January/139892.html >> >> I've adressed some of the review comments, most notably redoing the >> string buffer implementation quite a bit, and adding tests. >> The major change is that it is now decoupled from the parser >> and instead lives in /src/util/ as a utility for easier reuse. >> I've also closely followed the structure of our hash table and set >> so that it should be quite familiar for everyone. My implementation >> is also null terminated, while Vladislav's implementation was not. >> My reasoning behind this was that it's less fragile if someone where >> to access the contents of the buffer directly. I've also expanded >> with some different functions that allow us to do more stuff. >> Obviously this could be expanded upon further if need be. >> >> I've included some of Vladislav's less involved patches that >> I think we should get in as soon as possible. I've added Ian's >> r-b on the ones that he reviewed (I hope that's OK). The one >> patch that is missing from this series is the hand-written >> custom parser. While this was the thing that gave the biggest >> speed-up, unfortunately it's also a bit harder to review, >> so I've left that as a future exercise. >> >> I've run it through my complete shader-db and there's no changes >> or breakages, so that's encouraging. It amounts to about a 3% >> reduction in runtime and executed instructions on the shader-db run. >> It should be noted that the i965 backend is the primary bottleneck >> when running shader-db on my computer, so the numbers don't look all >> that impressive due to this. I'll see if I can come up with some >> numbers using the gallium noop driver. >> >> I have not done a very thorough job on testing that the output >> of the preprocessor/parser is the same after this series. >> However, there's no changes here that I believe should cause >> any issues, so I feel quite confident that this series should >> not cause any trouble. The original series did have some issues >> discovered by running a fuzzer over it, however I expect that >> was caused by the introduced hand-written fast-path scanner and >> associated macro substitutaion that are not a part of this series. >> >> CC: Vladislav Egorov >> CC: Ian Romanick >> >> Thomas Helland (5): >>util: Add a string buffer implementation >>util: Add tests for the string buffer >>glsl: Change the parser to use the string buffer >>glcpp: Use string_buffer for line continuation removal >>glcpp: Avoid unnecessary call to strlen >> >> Vladislav Egorov (4): >>glcpp: Avoid unnecessary strcmp() >>glcpp: Skip unnecessary line continuations removal >>ralloc: Use strnlen() inside of strncat() >>glcpp: Use Bloom filter before identifier search >> >> configure.ac| 1 + >> src/compiler/glsl/glcpp/glcpp-lex.l | 3 +- >> src/compiler/glsl/glcpp/glcpp-parse.y | 123 +--- >> src/compiler/glsl/glcpp/glcpp.h | 18 ++- >> src/compiler/glsl/glcpp/pp.c| 73 ++ >> src/util/Makefile.am| 3 +- >> src/util/Makefile.sources | 2 + >> src/util/ralloc.c
Re: [Mesa-dev] [PATCH 0/9][RFC] GLSL preprocessor/parser improvements
I suspect you forgot to test with a debug build, there is issues with the string buffer changes somewhere. ralloc.c:203: reralloc_size: Assertion `ralloc_parent(ptr) == ctx' failed. #0 0x770eb91f in raise () from /lib64/libc.so.6 #1 0x770ed51a in abort () from /lib64/libc.so.6 #2 0x770e3da7 in __assert_fail_base () from /lib64/libc.so.6 #3 0x770e3e52 in __assert_fail () from /lib64/libc.so.6 #4 0x7442c73c in reralloc_size (ctx=ctx@entry=0x7fffd40cb910, ptr=, size=) at ralloc.c:203 #5 0x744b519e in glcpp_preprocess (ralloc_ctx=ralloc_ctx@entry=0x7fffd40c9da0, shader=shader@entry=0x7fffea7cfcc8, info_log=info_log@entry=0x7fffd40ca058, extensions=extensions@entry=0x743ff070 char const*, int), glcpp_parser*, unsigned int, bool)>, state=state@entry=0x7fffd40c9da0, gl_ctx=gl_ctx@entry=0x7fffe85c7010) at glsl/glcpp/pp.c:246 #6 0x74402665 in _mesa_glsl_compile_shader (ctx=ctx@entry=0x7fffe85c7010, shader=shader@entry=0x7fffd40c8cc0, dump_ast=dump_ast@entry=false, dump_hir=dump_hir@entry=false, force_recompile=force_recompile@entry=false) at glsl/glsl_parser_extras.cpp:2065 On 22/05/17 06:49, Thomas Helland wrote: This patch series contains some of the work done by Vladislav in the beginning of March, that seems to have been forgotten. For reference, that series, with review comments, can be found here: https://lists.freedesktop.org/archives/mesa-dev/2017-January/139892.html I've adressed some of the review comments, most notably redoing the string buffer implementation quite a bit, and adding tests. The major change is that it is now decoupled from the parser and instead lives in /src/util/ as a utility for easier reuse. I've also closely followed the structure of our hash table and set so that it should be quite familiar for everyone. My implementation is also null terminated, while Vladislav's implementation was not. My reasoning behind this was that it's less fragile if someone where to access the contents of the buffer directly. I've also expanded with some different functions that allow us to do more stuff. Obviously this could be expanded upon further if need be. I've included some of Vladislav's less involved patches that I think we should get in as soon as possible. I've added Ian's r-b on the ones that he reviewed (I hope that's OK). The one patch that is missing from this series is the hand-written custom parser. While this was the thing that gave the biggest speed-up, unfortunately it's also a bit harder to review, so I've left that as a future exercise. I've run it through my complete shader-db and there's no changes or breakages, so that's encouraging. It amounts to about a 3% reduction in runtime and executed instructions on the shader-db run. It should be noted that the i965 backend is the primary bottleneck when running shader-db on my computer, so the numbers don't look all that impressive due to this. I'll see if I can come up with some numbers using the gallium noop driver. I have not done a very thorough job on testing that the output of the preprocessor/parser is the same after this series. However, there's no changes here that I believe should cause any issues, so I feel quite confident that this series should not cause any trouble. The original series did have some issues discovered by running a fuzzer over it, however I expect that was caused by the introduced hand-written fast-path scanner and associated macro substitutaion that are not a part of this series. CC: Vladislav Egorov CC: Ian Romanick Thomas Helland (5): util: Add a string buffer implementation util: Add tests for the string buffer glsl: Change the parser to use the string buffer glcpp: Use string_buffer for line continuation removal glcpp: Avoid unnecessary call to strlen Vladislav Egorov (4): glcpp: Avoid unnecessary strcmp() glcpp: Skip unnecessary line continuations removal ralloc: Use strnlen() inside of strncat() glcpp: Use Bloom filter before identifier search configure.ac| 1 + src/compiler/glsl/glcpp/glcpp-lex.l | 3 +- src/compiler/glsl/glcpp/glcpp-parse.y | 123 +--- src/compiler/glsl/glcpp/glcpp.h | 18 ++- src/compiler/glsl/glcpp/pp.c| 73 ++ src/util/Makefile.am| 3 +- src/util/Makefile.sources | 2 + src/util/ralloc.c | 7 +- src/util/string_buffer.c| 180 src/util/string_buffer.h| 75 ++ src/util/tests/string_buffer/Makefile.am| 34 + src/util/tests/string_buffer/append_and_print.c | 82 +++ 12 files changed, 505 insertions(+), 96 deletions(-) create mode 100644 src/util/string_buffer.c create mode 100644 src/util/string_buffer.h create mode 100644 src/uti
[Mesa-dev] [PATCH 0/9][RFC] GLSL preprocessor/parser improvements
This patch series contains some of the work done by Vladislav in the beginning of March, that seems to have been forgotten. For reference, that series, with review comments, can be found here: https://lists.freedesktop.org/archives/mesa-dev/2017-January/139892.html I've adressed some of the review comments, most notably redoing the string buffer implementation quite a bit, and adding tests. The major change is that it is now decoupled from the parser and instead lives in /src/util/ as a utility for easier reuse. I've also closely followed the structure of our hash table and set so that it should be quite familiar for everyone. My implementation is also null terminated, while Vladislav's implementation was not. My reasoning behind this was that it's less fragile if someone where to access the contents of the buffer directly. I've also expanded with some different functions that allow us to do more stuff. Obviously this could be expanded upon further if need be. I've included some of Vladislav's less involved patches that I think we should get in as soon as possible. I've added Ian's r-b on the ones that he reviewed (I hope that's OK). The one patch that is missing from this series is the hand-written custom parser. While this was the thing that gave the biggest speed-up, unfortunately it's also a bit harder to review, so I've left that as a future exercise. I've run it through my complete shader-db and there's no changes or breakages, so that's encouraging. It amounts to about a 3% reduction in runtime and executed instructions on the shader-db run. It should be noted that the i965 backend is the primary bottleneck when running shader-db on my computer, so the numbers don't look all that impressive due to this. I'll see if I can come up with some numbers using the gallium noop driver. I have not done a very thorough job on testing that the output of the preprocessor/parser is the same after this series. However, there's no changes here that I believe should cause any issues, so I feel quite confident that this series should not cause any trouble. The original series did have some issues discovered by running a fuzzer over it, however I expect that was caused by the introduced hand-written fast-path scanner and associated macro substitutaion that are not a part of this series. CC: Vladislav Egorov CC: Ian Romanick Thomas Helland (5): util: Add a string buffer implementation util: Add tests for the string buffer glsl: Change the parser to use the string buffer glcpp: Use string_buffer for line continuation removal glcpp: Avoid unnecessary call to strlen Vladislav Egorov (4): glcpp: Avoid unnecessary strcmp() glcpp: Skip unnecessary line continuations removal ralloc: Use strnlen() inside of strncat() glcpp: Use Bloom filter before identifier search configure.ac| 1 + src/compiler/glsl/glcpp/glcpp-lex.l | 3 +- src/compiler/glsl/glcpp/glcpp-parse.y | 123 +--- src/compiler/glsl/glcpp/glcpp.h | 18 ++- src/compiler/glsl/glcpp/pp.c| 73 ++ src/util/Makefile.am| 3 +- src/util/Makefile.sources | 2 + src/util/ralloc.c | 7 +- src/util/string_buffer.c| 180 src/util/string_buffer.h| 75 ++ src/util/tests/string_buffer/Makefile.am| 34 + src/util/tests/string_buffer/append_and_print.c | 82 +++ 12 files changed, 505 insertions(+), 96 deletions(-) create mode 100644 src/util/string_buffer.c create mode 100644 src/util/string_buffer.h create mode 100644 src/util/tests/string_buffer/Makefile.am create mode 100644 src/util/tests/string_buffer/append_and_print.c -- 2.13.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
