diff mbox series

[v1,1/7] tools/ocaml/Makefile: do not run ocamldep during make clean

Message ID 835ba84cf1fb7619fa6672d194aaf279795a5246.1659116941.git.edvin.torok@citrix.com (mailing list archive)
State New
Headers show
Series tools/ocaml code and build cleanups | expand

Commit Message

Edwin Torok July 29, 2022, 5:53 p.m. UTC
Trying to include .ocamldep.make will cause it to be generated if it
doesn't exist.
We do not want this during make clean: we would remove it anyway.

Speeds up make clean.

Before (measured on f732240fd3bac25116151db5ddeb7203b62e85ce, July 2022):
```
Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl

 Performance counter stats for 'make clean -j8 -s' (5 runs):

            4.2233 +- 0.0208 seconds time elapsed  ( +-  0.49% )
```

After:
```
perf stat -r 5 --null make clean -j8 -s

 Performance counter stats for 'make clean -j8 -s' (5 runs):

            2.7325 +- 0.0138 seconds time elapsed  ( +-  0.51% )
```

No functional change.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/ocaml/Makefile.rules | 2 ++
 1 file changed, 2 insertions(+)

Comments

Christian Lindig Aug. 1, 2022, 8:19 a.m. UTC | #1
On 29 Jul 2022, at 18:53, Edwin Török <edvin.torok@citrix.com<mailto:edvin.torok@citrix.com>> wrote:

Trying to include .ocamldep.make will cause it to be generated if it
doesn't exist.
We do not want this during make clean: we would remove it anyway.

Speeds up make clean.

Acked-by: Christian Lindig <christian.lindig@citrix.com<mailto:christian.lindig@citrix.com>>
Jan Beulich Aug. 3, 2022, 10:16 a.m. UTC | #2
On 29.07.2022 19:53, Edwin Török wrote:
> Trying to include .ocamldep.make will cause it to be generated if it
> doesn't exist.
> We do not want this during make clean: we would remove it anyway.
> 
> Speeds up make clean.
> 
> Before (measured on f732240fd3bac25116151db5ddeb7203b62e85ce, July 2022):
> ```
> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
> 
>  Performance counter stats for 'make clean -j8 -s' (5 runs):
> 
>             4.2233 +- 0.0208 seconds time elapsed  ( +-  0.49% )
> ```
> 
> After:
> ```
> perf stat -r 5 --null make clean -j8 -s
> 
>  Performance counter stats for 'make clean -j8 -s' (5 runs):
> 
>             2.7325 +- 0.0138 seconds time elapsed  ( +-  0.51% )
> ```
> 
> No functional change.
> 
> Signed-off-by: Edwin Török <edvin.torok@citrix.com>

I've committed this as is since it was acked and is an improvement
in any event, but ...

> --- a/tools/ocaml/Makefile.rules
> +++ b/tools/ocaml/Makefile.rules
> @@ -44,8 +44,10 @@ META: META.in
>  
>  ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))
>  
> +ifneq ($(MAKECMDGOALS),clean)
>  .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
>  	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
> +endif

... what about the distclean goal?

Jan
Edwin Torok Aug. 3, 2022, 10:24 a.m. UTC | #3
> On 3 Aug 2022, at 11:16, Jan Beulich <jbeulich@suse.com> wrote:
> 
> On 29.07.2022 19:53, Edwin Török wrote:
>> Trying to include .ocamldep.make will cause it to be generated if it
>> doesn't exist.
>> We do not want this during make clean: we would remove it anyway.
>> 
>> Speeds up make clean.
>> 
>> Before (measured on f732240fd3bac25116151db5ddeb7203b62e85ce, July 2022):
>> ```
>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>> 
>> Performance counter stats for 'make clean -j8 -s' (5 runs):
>> 
>>            4.2233 +- 0.0208 seconds time elapsed  ( +-  0.49% )
>> ```
>> 
>> After:
>> ```
>> perf stat -r 5 --null make clean -j8 -s
>> 
>> Performance counter stats for 'make clean -j8 -s' (5 runs):
>> 
>>            2.7325 +- 0.0138 seconds time elapsed  ( +-  0.51% )
>> ```
>> 
>> No functional change.
>> 
>> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
> 
> I've committed this as is since it was acked and is an improvement
> in any event, but ...
> 
>> --- a/tools/ocaml/Makefile.rules
>> +++ b/tools/ocaml/Makefile.rules
>> @@ -44,8 +44,10 @@ META: META.in
>> 
>> ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))
>> 
>> +ifneq ($(MAKECMDGOALS),clean)
>> .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
>> 	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
>> +endif
> 
> ... what about the distclean goal?


Thanks for the suggestion, I see other Makefiles using 'findstring clean', would that be appropriate here?

Something like this perhaps?

From 53cbf81a9c7d5090443b5fe5de37a47118ac53d8 Mon Sep 17 00:00:00 2001
Message-Id: <53cbf81a9c7d5090443b5fe5de37a47118ac53d8.1659522178.git.edvin.torok@citrix.com>
From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= <edvin.torok@citrix.com>
To: xen-devel@lists.xenproject.org
Cc: Christian Lindig <christian.lindig@citrix.com>
Cc: David Scott <dave@recoil.org>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Date: Wed, 3 Aug 2022 11:21:46 +0100
Subject: [PATCH] tools/ocaml/Makefile.rules: do not run ocamldep on distclean
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
---
 tools/ocaml/Makefile.rules | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/ocaml/Makefile.rules b/tools/ocaml/Makefile.rules
index d368308d9b..c1c5dd3b39 100644
--- a/tools/ocaml/Makefile.rules
+++ b/tools/ocaml/Makefile.rules
@@ -44,7 +44,7 @@ META: META.in

 ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))

-ifneq ($(MAKECMDGOALS),clean)
+ifeq (,$(findstring clean,$(MAKECMDGOALS)))
 .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
 	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
 endif
--
2.34.1
Jan Beulich Aug. 3, 2022, 10:47 a.m. UTC | #4
On 03.08.2022 12:24, Edwin Torok wrote:
> 
> 
>> On 3 Aug 2022, at 11:16, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 29.07.2022 19:53, Edwin Török wrote:
>>> Trying to include .ocamldep.make will cause it to be generated if it
>>> doesn't exist.
>>> We do not want this during make clean: we would remove it anyway.
>>>
>>> Speeds up make clean.
>>>
>>> Before (measured on f732240fd3bac25116151db5ddeb7203b62e85ce, July 2022):
>>> ```
>>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>>> Parsing /home/edwin/xen2/tools/ocaml/libs/xl/../../../../tools/libs/light/libxl_types.idl
>>>
>>> Performance counter stats for 'make clean -j8 -s' (5 runs):
>>>
>>>            4.2233 +- 0.0208 seconds time elapsed  ( +-  0.49% )
>>> ```
>>>
>>> After:
>>> ```
>>> perf stat -r 5 --null make clean -j8 -s
>>>
>>> Performance counter stats for 'make clean -j8 -s' (5 runs):
>>>
>>>            2.7325 +- 0.0138 seconds time elapsed  ( +-  0.51% )
>>> ```
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Edwin Török <edvin.torok@citrix.com>
>>
>> I've committed this as is since it was acked and is an improvement
>> in any event, but ...
>>
>>> --- a/tools/ocaml/Makefile.rules
>>> +++ b/tools/ocaml/Makefile.rules
>>> @@ -44,8 +44,10 @@ META: META.in
>>>
>>> ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))
>>>
>>> +ifneq ($(MAKECMDGOALS),clean)
>>> .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
>>> 	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
>>> +endif
>>
>> ... what about the distclean goal?
> 
> 
> Thanks for the suggestion, I see other Makefiles using 'findstring clean', would that be appropriate here?

Hmm, not sure this couldn't end up suppressing the rul when it's
needed. How about "ifeq ($(filter-out %clean,$(MAKECMDGOALS)),)"?
(Off the top of my head I don't recall whether this may additionally
need wrapping in $(strip ...).)

Jan
Anthony PERARD Aug. 3, 2022, 10:57 a.m. UTC | #5
On Wed, Aug 03, 2022 at 10:24:26AM +0000, Edwin Torok wrote:
> 
> -ifneq ($(MAKECMDGOALS),clean)
> +ifeq (,$(findstring clean,$(MAKECMDGOALS)))

I think it would be better with $(filter-out,):

    ifeq (,$(filter-out %clean,$(MAKECMDGOALS)))

>  .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
>  	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)

Also, don't hide this rule, instead, hide the "-include", there is no
need to have make waist time trying to find a rule to make
".ocamldep.make" and failing when not needed.

>  endif
> --
> 2.34.1
Jan Beulich Aug. 3, 2022, 11:58 a.m. UTC | #6
On 03.08.2022 12:57, Anthony PERARD wrote:
> On Wed, Aug 03, 2022 at 10:24:26AM +0000, Edwin Torok wrote:
>>
>> -ifneq ($(MAKECMDGOALS),clean)
>> +ifeq (,$(findstring clean,$(MAKECMDGOALS)))
> 
> I think it would be better with $(filter-out,):
> 
>     ifeq (,$(filter-out %clean,$(MAKECMDGOALS)))
> 
>>  .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
>>  	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
> 
> Also, don't hide this rule, instead, hide the "-include", there is no
> need to have make waist time trying to find a rule to make
> ".ocamldep.make" and failing when not needed.

Hmm, this sounds like I should be reverting the commit?

Jan
Anthony PERARD Aug. 3, 2022, 12:42 p.m. UTC | #7
On Wed, Aug 03, 2022 at 01:58:34PM +0200, Jan Beulich wrote:
> On 03.08.2022 12:57, Anthony PERARD wrote:
> > On Wed, Aug 03, 2022 at 10:24:26AM +0000, Edwin Torok wrote:
> >>
> >> -ifneq ($(MAKECMDGOALS),clean)
> >> +ifeq (,$(findstring clean,$(MAKECMDGOALS)))
> > 
> > I think it would be better with $(filter-out,):
> > 
> >     ifeq (,$(filter-out %clean,$(MAKECMDGOALS)))
> > 
> >>  .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
> >>  	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
> > 
> > Also, don't hide this rule, instead, hide the "-include", there is no
> > need to have make waist time trying to find a rule to make
> > ".ocamldep.make" and failing when not needed.
> 
> Hmm, this sounds like I should be reverting the commit?

Well, it's not exactly an issue as there isn't any alternative rules,
and make is told to ignore failures to make ".ocamldep.make"; so `make
clean` and other targets still works as expected. Just a follow-up patch
would be fine I think.
diff mbox series

Patch

diff --git a/tools/ocaml/Makefile.rules b/tools/ocaml/Makefile.rules
index 7e4db457a1..d368308d9b 100644
--- a/tools/ocaml/Makefile.rules
+++ b/tools/ocaml/Makefile.rules
@@ -44,8 +44,10 @@  META: META.in
 
 ALL_OCAML_OBJ_SOURCES=$(addsuffix .ml, $(ALL_OCAML_OBJS))
 
+ifneq ($(MAKECMDGOALS),clean)
 .ocamldep.make: $(ALL_OCAML_OBJ_SOURCES) Makefile $(OCAML_TOPLEVEL)/Makefile.rules
 	$(call quiet-command, $(OCAMLDEP) $(ALL_OCAML_OBJ_SOURCES) *.mli $o,MLDEP,)
+endif
 
 clean: $(CLEAN_HOOKS)
 	$(Q)rm -f .*.d *.o *.so *.a *.cmo *.cmi *.cma *.cmx *.cmxa *.annot *.spot *.spit $(LIBS) $(PROGRAMS) $(GENERATED_FILES) .ocamldep.make META