| Message ID | 20220801204227.2993126-1-yann.morin.1998@free.fr |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [PATCHv2] Makefile: fix show-vars for good this time | expand |
Hi Yann, On 8/1/22 22:42, Yann E. MORIN wrote: > Commit 5c54c3ef3db2 (Makefile: workaround make 4.3 issue for 'printvars > and 'show-vars') did not fully fix the show-vars case, which still > segfaults. > > Overall, show-vars generates a JSON blurb. That is supposed to be > machine-readable, so we do not care that the variables are sorted, so > we get rid of it to (slightly) simplify the code. > > Then, we currently iterate twice on the list of variables: the first one > to filter-out the 'internal' variables, and the second one to filter > only the variables matching the pattern. We can do away by iterating > only once, and applying both filters at once. > > Since we now have an 'and' condition, we can take advantage of it: when > none of the items in $(and) are empty, $(and) evaluates to the last > item, while it evaluates to empty if any of the items is empty. So we > can coalesce the $(if) and $(and) together: $(if $(and a,b),c) is > equivalent to: $(and a,b,c) ; this gains us one parentheses depth. > > Finally, the cause for the segfault is an overly-long call to $(info). > Reducing that is not easy: we want to call clean-json on the whole of > the JSON blurb, so we can't emit the individual variables one by one, or > the trailing comma would not be trimmed away. > > So, we go crazy: we just output each word from clean-json with $(info). > > We can do that, because mk-json-str transforms all spaces in a string > to an escaped UTF-8 sequence, so we will never have spaces in values; > the keys are the variables, so they won't have spaces either; spaces in > the rest of the JSON blurb are totally optional, so we don't care how > many there are. We know there are spaces, because we explicitly > introduce some (after "expanded" or "raw", for example), so we should > never hit a too-big word for $(info) to print. > > Thanks to Henri for the suggestion to push $(info) further inside the > macro. > > Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr> > Cc: Roosen Henri <Henri.Roosen@ginzinger.com> > Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > Tested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> Upon request, "time efficiency" regression test on old make: From within debian:buster container (make 4.2.1), from master 2a592f0b2a400825a95e19113ce996bbc5c431d7: # make raspberrypi4_defconfig # time make show-vars > foo.json real 0m19.001s user 0m15.828s sys 0m3.305s # time make printvars VARS='%' > foo.txt real 0m16.271s user 0m13.056s sys 0m3.360s # ls -l foo.json foo.txt -rw-r--r--. 1 qschulz qschulz 62448149 Aug 2 09:06 foo.json -rw-r--r--. 1 qschulz qschulz 32813206 Aug 2 09:07 foo.txt From within debian:buster container (make 4.2.1), with this patch NOT applied and 5c54c3ef3d commit reverted: # make raspberrypi4_defconfig # time make show-vars > foo.json real 0m24.325s user 0m19.617s sys 0m4.905s # time make printvars VARS='%' > foo.txt real 0m16.223s user 0m12.895s sys 0m3.469s # ls -l foo.json foo.txt -rw-r--r--. 1 qschulz qschulz 62448149 Aug 2 09:04 foo.json -rw-r--r--. 1 qschulz qschulz 32813206 Aug 2 09:09 foo.txt From within debian:buster container (make 4.2.1), with this patch applied: # make raspberrypi4_defconfig # time make show-vars > foo.json real 0m22.634s user 0m17.542s sys 0m5.202s # time make printvars VARS='%' > foo.txt real 0m16.829s user 0m13.381s sys 0m3.579s # ls -l foo.json foo.txt -rw-r--r--. 1 qschulz qschulz 62448149 Aug 2 09:02 foo.json -rw-r--r--. 1 qschulz qschulz 32813206 Aug 2 09:14 foo.txt I wouldn't trust too much the reports from the time tool because I had some runs where it was twice longer for some reason. In short, it does not seem to impact too much time-wise. Thanks, Quentin
On Mon, 1 Aug 2022 22:42:27 +0200 "Yann E. MORIN" <yann.morin.1998@free.fr> wrote: > Commit 5c54c3ef3db2 (Makefile: workaround make 4.3 issue for 'printvars > and 'show-vars') did not fully fix the show-vars case, which still > segfaults. > > Overall, show-vars generates a JSON blurb. That is supposed to be > machine-readable, so we do not care that the variables are sorted, so > we get rid of it to (slightly) simplify the code. > > Then, we currently iterate twice on the list of variables: the first one > to filter-out the 'internal' variables, and the second one to filter > only the variables matching the pattern. We can do away by iterating > only once, and applying both filters at once. > > Since we now have an 'and' condition, we can take advantage of it: when > none of the items in $(and) are empty, $(and) evaluates to the last > item, while it evaluates to empty if any of the items is empty. So we > can coalesce the $(if) and $(and) together: $(if $(and a,b),c) is > equivalent to: $(and a,b,c) ; this gains us one parentheses depth. > > Finally, the cause for the segfault is an overly-long call to $(info). > Reducing that is not easy: we want to call clean-json on the whole of > the JSON blurb, so we can't emit the individual variables one by one, or > the trailing comma would not be trimmed away. > > So, we go crazy: we just output each word from clean-json with $(info). > > We can do that, because mk-json-str transforms all spaces in a string > to an escaped UTF-8 sequence, so we will never have spaces in values; > the keys are the variables, so they won't have spaces either; spaces in > the rest of the JSON blurb are totally optional, so we don't care how > many there are. We know there are spaces, because we explicitly > introduce some (after "expanded" or "raw", for example), so we should > never hit a too-big word for $(info) to print. > > Thanks to Henri for the suggestion to push $(info) further inside the > macro. > > Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr> > Cc: Roosen Henri <Henri.Roosen@ginzinger.com> > Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > > --- > Changes v1 -> v2: > - coalesce $(if) with $(and) > - don't output a trailing $(space) when calling $(info) > --- > Makefile | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) Applied to master, thanks. Thomas
>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes: > Commit 5c54c3ef3db2 (Makefile: workaround make 4.3 issue for 'printvars > and 'show-vars') did not fully fix the show-vars case, which still > segfaults. > Overall, show-vars generates a JSON blurb. That is supposed to be > machine-readable, so we do not care that the variables are sorted, so > we get rid of it to (slightly) simplify the code. > Then, we currently iterate twice on the list of variables: the first one > to filter-out the 'internal' variables, and the second one to filter > only the variables matching the pattern. We can do away by iterating > only once, and applying both filters at once. > Since we now have an 'and' condition, we can take advantage of it: when > none of the items in $(and) are empty, $(and) evaluates to the last > item, while it evaluates to empty if any of the items is empty. So we > can coalesce the $(if) and $(and) together: $(if $(and a,b),c) is > equivalent to: $(and a,b,c) ; this gains us one parentheses depth. > Finally, the cause for the segfault is an overly-long call to $(info). > Reducing that is not easy: we want to call clean-json on the whole of > the JSON blurb, so we can't emit the individual variables one by one, or > the trailing comma would not be trimmed away. > So, we go crazy: we just output each word from clean-json with $(info). > We can do that, because mk-json-str transforms all spaces in a string > to an escaped UTF-8 sequence, so we will never have spaces in values; > the keys are the variables, so they won't have spaces either; spaces in > the rest of the JSON blurb are totally optional, so we don't care how > many there are. We know there are spaces, because we explicitly > introduce some (after "expanded" or "raw", for example), so we should > never hit a too-big word for $(info) to print. > Thanks to Henri for the suggestion to push $(info) further inside the > macro. > Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> > Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr> > Cc: Roosen Henri <Henri.Roosen@ginzinger.com> > Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > --- > Changes v1 -> v2: > - coalesce $(if) with $(and) > - don't output a trailing $(space) when calling $(info) Committed to 2022.05.x and 2022.02.x, thanks.
diff --git a/Makefile b/Makefile index a743e42f91..07b594ea8a 100644 --- a/Makefile +++ b/Makefile @@ -1073,17 +1073,24 @@ printvars: show-vars: VARS?=% show-vars: @: - $(info $(call clean-json, { \ + $(foreach i, \ + $(call clean-json, { \ $(foreach V, \ - $(sort $(foreach X, $(.VARIABLES), $(filter $(VARS),$(X)))), \ - $(if $(filter-out environment% default automatic, $(origin $V)), \ + $(.VARIABLES), \ + $(and $(filter $(VARS),$(V)) \ + , \ + $(filter-out environment% default automatic, $(origin $V)) \ + , \ "$V": { \ "expanded": $(call mk-json-str,$($V))$(comma) \ "raw": $(call mk-json-str,$(value $V)) \ }$(comma) \ ) \ ) \ - } )) + } ) \ + , \ + $(info $(i)) \ + ) .PHONY: clean clean:
Commit 5c54c3ef3db2 (Makefile: workaround make 4.3 issue for 'printvars and 'show-vars') did not fully fix the show-vars case, which still segfaults. Overall, show-vars generates a JSON blurb. That is supposed to be machine-readable, so we do not care that the variables are sorted, so we get rid of it to (slightly) simplify the code. Then, we currently iterate twice on the list of variables: the first one to filter-out the 'internal' variables, and the second one to filter only the variables matching the pattern. We can do away by iterating only once, and applying both filters at once. Since we now have an 'and' condition, we can take advantage of it: when none of the items in $(and) are empty, $(and) evaluates to the last item, while it evaluates to empty if any of the items is empty. So we can coalesce the $(if) and $(and) together: $(if $(and a,b),c) is equivalent to: $(and a,b,c) ; this gains us one parentheses depth. Finally, the cause for the segfault is an overly-long call to $(info). Reducing that is not easy: we want to call clean-json on the whole of the JSON blurb, so we can't emit the individual variables one by one, or the trailing comma would not be trimmed away. So, we go crazy: we just output each word from clean-json with $(info). We can do that, because mk-json-str transforms all spaces in a string to an escaped UTF-8 sequence, so we will never have spaces in values; the keys are the variables, so they won't have spaces either; spaces in the rest of the JSON blurb are totally optional, so we don't care how many there are. We know there are spaces, because we explicitly introduce some (after "expanded" or "raw", for example), so we should never hit a too-big word for $(info) to print. Thanks to Henri for the suggestion to push $(info) further inside the macro. Reported-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr> Cc: Roosen Henri <Henri.Roosen@ginzinger.com> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com> --- Changes v1 -> v2: - coalesce $(if) with $(and) - don't output a trailing $(space) when calling $(info) --- Makefile | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)