Re: [PATCH] maint.mk: allow fine-grained syntax-check exclusion via Make variables
Jim Meyering wrote: Jim Meyering wrote: Reuben Thomas wrote: make syntax-check is complaining about space-tabs (sc_space_tab) in a sort of file where this is perfectly permissable: a .diff file. Why do I have a diff file in version control? Because I'm patching gnulib. Of course, I can add this to VC_LIST_ALWAYS_EXCLUDE_REGEX, but maybe .diff files should be excluded from this check anyway? They're expected only in .diff files for which the original has context lines that start with a TAB. For that reason (in gnulib, that is only a very small fraction of all files), I think it's slightly better to let those who need it add a line like this to a file named .x-sc_space_tab ^gl/lib/.*\.c\.diff$ However, I find that adding a whole new .x-sc_* file just to exempt an exceptional source file from one of the many syntax checks is a disproportionate burden. It has always bothered me to do that. So finally, here's a proposed maint.mk patch to implement a better way, followed by the change induced in coreutils where I remove its 24 .x-sc_* files, replacing them with just 30 lines at the end of cfg.mk: Notes on the naming of these new exception-specifying variables: - the resulting variable names are rather long. I erred on the side of being too descriptive. They're going to be used at most once, then probably forgotten forever. - I don't like the fact that they have a common *suffix* rather than a common prefix. That's just what I did in the first cut. They do have a common sc_ suffix, so maybe that's ok, but the long common part, -exclude_file_name_regexp is at the end, and that makes the list in cfg.mk harder to read, so I'm leaning towards reversing, i.e., changing this sc_space_tab-exclude_file_name_regexp = \ to this _exclude_file_name_regexp--sc_space_tab = \ Note the leading underscore and two hyphens. The former to make it less likely to collied with application names, and the latter to make it clearer where the long common prefix ends and the variable suffix starts. Plus I'll have to split the long line 10 lines down: I've done the above and have just pushed this change. Thanks for inspiring me to do this, Reuben. ... Subject: [PATCH] maint.mk: allow fine-grained syntax-check exclusion via Make variables Before, you would have had to create one .x-sc_ file per rule in order to exempt offending files. Now, you may instead use a Make variable -- usually defined in cfg.mk -- whose name identifies the affected rule. * top/maint.mk (_sc_excl): Define. (VC_LIST_EXCEPT): Use it to exclude names on a per-rule basis. (_sc_search_regexp): When not using VC_LIST_EXCEPT, exclude here, too. FYI, here's the corresponding patch to coreutils. It's not often that we can remove so many files. This brings the number of VC'd files back below 1000 (now it's 984). From 8e4e1d484f88502dbe9336050232a5f90e0b68d4 Mon Sep 17 00:00:00 2001 From: Jim Meyering meyer...@redhat.com Date: Mon, 14 Mar 2011 14:26:38 +0100 Subject: [PATCH] maint: stop using .x-sc_* files to list syntax-check exemptions Instead, use the brand new mechanism with which you merely use a variable (derived from the rule name) defined in cfg.mk to an ERE matching the exempted file names. * gnulib: Update to latest, to get maint.mk that implements this. * Makefile.am (syntax_check_exceptions): Remove variable. (EXTRA_DIST): Remove use of the variable. * cfg.mk (sc_x_sc_dist_check): Remove rule, no longer useful. (exclude_file_name_regexp--sc_space_tab): Define variable. (exclude_file_name_regexp--sc_bindtextdomain): Likewise. (exclude_file_name_regexp--sc_unmarked_diagnostics): Likewise. (exclude_file_name_regexp--sc_error_message_uppercase): Likewise. (exclude_file_name_regexp--sc_trailing_blank): Likewise. (exclude_file_name_regexp--sc_system_h_headers): Likewise. (exclude_file_name_regexp--sc_require_config_h_first): Likewise. (exclude_file_name_regexp--sc_require_config_h): Likewise. (exclude_file_name_regexp--sc_po_check): Likewise. (exclude_file_name_regexp--sc_prohibit_always-defined_macros): Likewise. (exclude_file_name_regexp--sc_prohibit_empty_lines_at_EOF): Likewise. (exclude_file_name_regexp--sc_program_name): Likewise. (exclude_file_name_regexp--sc_file_system): Likewise. (exclude_file_name_regexp--sc_prohibit_always_true_header_tests): Likewise. (exclude_file_name_regexp--sc_prohibit_fail_0): Likewise. (exclude_file_name_regexp--sc_prohibit_atoi_atof): Likewise. (exclude_file_name_regexp--sc_prohibit_tab_based_indentation): Likewise. (exclude_file_name_regexp--sc_prohibit_stat_st_blocks): Likewise. * configure.ac [whether localtime caches TZ]: Use return 0/1, not exit (0/1) to avoid triggering a sc_prohibit_magic_number_exit failure. * .x-sc_GPL_version: Remove file. * .x-sc_bindtextdomain: Likewise. * .x-sc_error_message_uppercase: Likewise. * .x-sc_file_system: Likewise. * .x-sc_obsolete_symbols: Likewise. * .x-sc_po_check: Likewise. *
Re: parallel sort at fault? [Re: [PATCH] tests: avoid gross inefficiency...
Pádraig Brady wrote: I've not fully analyzed this yet, and I'm not saying it's wrong, but the above change seems to have a large effect on thread creation when smaller buffers are used (you hinted previously that being less aggressive with the amount of mem used by default might be appropriate, and I agree). Anyway with the above I seem to need a buffer size more than 10M to have any threads created at all. Testing the original 4 lines heuristic with the following, shows: (note I only get 4 threads after 4M of input, not 7 for 16 lines as indicated in NEWS). $ for i in $(seq 30); do j=$((2$i)) yes | head -n$j t.sort strace -c -e clone sort --parallel=16 t.sort -o /dev/null 21 | join --nocheck-order -a1 -o1.4,1.5 - /dev/null | sed -n s/\([0-9]*\) clone/$j\t\1/p done 4 1 8 2 16 3 32 4 64 4 128 4 ... 1048576 4 2097152 4 4194304 8 8388608 16 When I restrict the buffer size with '-S 1M', many more threads are created (a max of 16 in parallel with the above command) 4 1 8 2 16 3 32 4 64 4 128 4 256 4 512 4 10244 20484 40964 81924 16384 8 32768 12 65536 24 131072 44 262144 84 524288 167 1048576 332 2097152 660 4194304 1316 8388608 2628 After increasing the heuristic to 128K, I get _no_ threads until -S 10M and this seems to be independent of line length. Thanks for investigating that. Could strace -c -e clone be doing something unexpected? When I run this (without my patch), it would use 8 threads: seq 16 in; strace -ff -o k ./sort --parallel=16 in -o /dev/null since it created eight k.PID files: $ ls -1 k.*|wc -l 8 Now, for such a small file, it does not call clone at all.
Re: parallel sort at fault? [Re: [PATCH] tests: avoid gross inefficiency...
On 16/03/11 12:07, Jim Meyering wrote: Pádraig Brady wrote: I've not fully analyzed this yet, and I'm not saying it's wrong, but the above change seems to have a large effect on thread creation when smaller buffers are used (you hinted previously that being less aggressive with the amount of mem used by default might be appropriate, and I agree). Anyway with the above I seem to need a buffer size more than 10M to have any threads created at all. Testing the original 4 lines heuristic with the following, shows: (note I only get 4 threads after 4M of input, not 7 for 16 lines as indicated in NEWS). $ for i in $(seq 30); do j=$((2$i)) yes | head -n$j t.sort strace -c -e clone sort --parallel=16 t.sort -o /dev/null 21 | join --nocheck-order -a1 -o1.4,1.5 - /dev/null | sed -n s/\([0-9]*\) clone/$j\t\1/p done 4 1 8 2 16 3 32 4 64 4 128 4 ... 1048576 4 2097152 4 4194304 8 8388608 16 When I restrict the buffer size with '-S 1M', many more threads are created (a max of 16 in parallel with the above command) 4 1 8 2 16 3 32 4 64 4 128 4 256 4 512 4 10244 20484 40964 81924 16384 8 32768 12 65536 24 131072 44 262144 84 524288 167 1048576 332 2097152 660 4194304 1316 8388608 2628 After increasing the heuristic to 128K, I get _no_ threads until -S 10M and this seems to be independent of line length. Thanks for investigating that. Could strace -c -e clone be doing something unexpected? When I run this (without my patch), it would use 8 threads: seq 16 in; strace -ff -o k ./sort --parallel=16 in -o /dev/null since it created eight k.PID files: $ ls -1 k.*|wc -l 8 Now, for such a small file, it does not call clone at all. Oops, yep I forget to add -f to strace. So NEWS is correct. # SUBTHREAD_LINES_HEURISTIC = 4 $ for i in $(seq 22); do j=$((2$i)) yes | head -n$j t.sort strace -f -c -e clone ./sort --parallel=16 t.sort -o /dev/null 21 | join --nocheck-order -a1 -o1.4,1.5 - /dev/null | sed -n s/\([0-9]*\) clone/$j\t\1/p done 4 1 8 3 16 7 32 15 64 15 128 15 256 15 512 15 102415 204815 409615 819215 16384 15 32768 15 65536 15 131072 15 262144 15 524288 15 1048576 15 2097152 15 4194304 30 8388608 45 # As above, but add -S1M option to sort 4 1 8 3 16 7 32 15 64 15 128 15 256 15 512 15 102415 204815 409615 819215 16384 30 32768 45 65536 90 131072 165 262144 315 524288 622 1048576 1245 2097152 2475 4194304 4935 8388608 9855 With SUBTHREAD_LINES_HEURISTIC=128k and -S1M option to sort we get no threads as nlines never gets above 12787 (there looks to be around 80 bytes overhead per line?). Only when -S = 12M do we get nlines high enough to create threads. cheers, Pádraig.
Re: parallel sort at fault? [Re: [PATCH] tests: avoid gross inefficiency...
On 16/03/11 15:32, Jim Meyering wrote:
Pádraig Brady wrote:
With SUBTHREAD_LINES_HEURISTIC=128k and -S1M option to sort we get no
threads as
nlines never gets above 12787 (there looks to be around 80 bytes overhead
per line?).
Only when -S = 12M do we get nlines high enough to create threads.
Thanks for pursuing this.
Here's a proposed patch to address the other problem.
It doesn't have much of an effect (any?) on your
issue when using very little memory, but when a sort user
specifies -S1M, I think they probably want to avoid the
expense (memory) of going multi-threaded.
What do you think?
-#define INPUT_FILE_SIZE_GUESS (1024 * 1024)
+#define INPUT_FILE_SIZE_GUESS (128 * 1024)
This does seem a bit like whack-a-mole
but at least we're lining them up better.
The above gives reasonable threading by default,
while reducing the large upfront malloc.
$ for len in 1 79; do
for i in $(seq 22); do
lines=$((2$i))
yes $(printf %${len}s)| head -n$lines t.sort
strace -f -c -e clone ./sort --parallel=16 t.sort -o /dev/null 21 |
join --nocheck-order -a1 -o1.4,1.5 - /dev/null |
sed -n s/\([0-9]*\) clone/$lines\t\1/p
done
done
#lines threads (2 byte lines)
--
131072 1
262144 3
524288 7
1048576 15
2097152 15
4194304 15
8388608 15
#lines threads (80 byte lines)
--
131072 1
262144 3
524288 7
1048576 15
2097152 15
4194304 22
8388608 60
cheers,
Pádraig.
Re: [PATCH] copy: adjust fiemap handling of sparse files
On 16/03/11 19:18, Mike Frysinger wrote: On Wednesday, March 16, 2011 11:26:40 Pádraig Brady wrote: On 14/02/11 06:05, Jim Meyering wrote: Pádraig Brady wrote: For my own reference, I can probably skip performance tests on (older) btrfs by checking `filefrag` output. Also in `cp`, if we see an unwritten extent we should probably create a hole rather than an empty allocation by default. It's better to decrease file allocation than increase it. Makes sense. Thinking a bit more about it, I'm not sure I should do the above, as one would be more surprised if cp by default introduced holes into a copy of a fully allocated file. The previously applied patch changes behavior on BTRFS on Fedora 14, in that it will convert a file with holes to a fully allocated one with zeros. But that is due to an already fixed bug in BTRFS where it reports holes as empty extents. I'm inclined to leave this as is, because this stuff is tricky enough, without introducing work arounds for buggy file systems. There is no data loss in this case, just bigger files (which one can avoid with cp --sparse=always). Also it will not be common to overlap future coreutils releases, with older BTRFS implementations. well, in the bug report i was working with, we were seeing data loss. i wonder if it'd be possible to detect the fs/kernel version and error out when versions are found that are known to be broken ? That was a different issue. It would be nice, but I don't think it would be practical to try and detect and work around such file system issues in cp. There are always going to be teething problems with a large body of new logic, so I think the best approach with file systems is to increase trust in the gradually over time. Personally I'd consider BTRFS for my backup drive at present, which it should be particularly good at given its snapshot capabilities. cheers, Pádraig.
