[PATCH] scripts: avoid redundant re-reading makefiles.

[PATCH] scripts: avoid redundant re-reading makefiles.

[Alexander Pantyukhin]

From: alexpantyukhin <apantykhin@gmail.com>

The main goal of the patch is avoid redundant re-reading makefiles 
if these were read before. This is performance improvement.

Additional code improvements are using 'with' when read file and check
if file exists.

Signed-off-by: Alexander Pantyukhin <apantykhin@gmail.com>
---
 scripts/generate_rust_analyzer.py | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/scripts/generate_rust_analyzer.py b/scripts/generate_rust_analyzer.py
index ecc7ea9a4dcf..e949f32b8123 100755
--- a/scripts/generate_rust_analyzer.py
+++ b/scripts/generate_rust_analyzer.py
@@ -98,13 +98,25 @@ def generate_crates(srctree, objtree, sysroot_src):
     # Then, the rest outside of `rust/`.
     #
     # We explicitly mention the top-level folders we want to cover.
+    makefile_path_content = {}
     for folder in ("samples", "drivers"):
         for path in (srctree / folder).rglob("*.rs"):
             logging.info("Checking %s", path)
             name = path.name.replace(".rs", "")
 
             # Skip those that are not crate roots.
-            if f"{name}.o" not in open(path.parent / "Makefile").read():
+            makefile_path = path.parent / "Makefile"
+
+            if makefile_path not in makefile_path_content:
+                if makefile_path.is_file():
+                    with open(makefile_path) as makefile:
+                        makefile_path_content[makefile_path] = makefile.read()
+
+                else:
+                    # If file is missing we consider it as empty-content file.
+                    makefile_path_content[makefile_path] = ""
+
+            if f"{name}.o" not in makefile_path_content[makefile_path]:
                 continue
 
             logging.info("Adding %s", name)
-- 
2.25.1

Re: [PATCH] scripts: avoid redundant re-reading makefiles.

[Miguel Ojeda]

Hi Alex,

The commit message is looking better, thank you. Please see comments
inline below.

On Wed, Jan 4, 2023 at 5:04 PM Alexander Pantyukhin
<apantykhin@gmail.com> wrote:
>
> The main goal of the patch is avoid redundant re-reading makefiles
> if these were read before. This is performance improvement.

Like it was mentioned in v1, could you please give a rough estimate
about what is the improvement from old to new? (in particular, without
the other changes). That way, others can get an idea whether the code
complexity is worth it or not.

> Additional code improvements are using 'with' when read file and check
> if file exists.

Note that others submitted an improvement for the missing file case as
we discussed in v1, so it would be better if they submitted their own
patches for those. I am Cc'ing them so that they have a chance to be
aware of this patch and potentially submit theirs.

In any case, their approach seems more idiomatic for Python [1], so I
think we should do it that way.

Finally, the title of this commit should probably start with the
"scripts: generate_rust_analyzer: " prefix, otherwise it is a bit too
broad.

Thanks!

Cheers,
Miguel

[1] https://docs.python.org/3.11/glossary.html#term-EAFP

Re: [PATCH] scripts: avoid redundant re-reading makefiles.

[Alexander Pantyukhin]

Hi Miguel.
Please see my comments below.

чт, 5 янв. 2023 г. в 22:44, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>:
>
> Hi Alex,
>
> The commit message is looking better, thank you. Please see comments
> inline below.
>
> On Wed, Jan 4, 2023 at 5:04 PM Alexander Pantyukhin
> <apantykhin@gmail.com> wrote:
> >
> > The main goal of the patch is avoid redundant re-reading makefiles
> > if these were read before. This is performance improvement.
>
> Like it was mentioned in v1, could you please give a rough estimate
> about what is the improvement from old to new? (in particular, without
> the other changes). That way, others can get an idea whether the code
> complexity is worth it or not.

I checked files now. It seems only one place in /samples  has 2 files
with 1 makefile in the parent folder. So it looks like the patch is
untimely for now.

>
> > Additional code improvements are using 'with' when read file and check
> > if file exists.
>
> Note that others submitted an improvement for the missing file case as
> we discussed in v1, so it would be better if they submitted their own
> patches for those. I am Cc'ing them so that they have a chance to be
> aware of this patch and potentially submit theirs.
>
> In any case, their approach seems more idiomatic for Python [1], so I
> think we should do it that way.
>
> Finally, the title of this commit should probably start with the
> "scripts: generate_rust_analyzer: " prefix, otherwise it is a bit too
> broad.

I see, thanks. Still have a question about using "with". Is it reasonable?

>
> Thanks!
>
> Cheers,
> Miguel
>
> [1] https://docs.python.org/3.11/glossary.html#term-EAFP

Thank you!
Best, Alex.

Re: [PATCH] scripts: avoid redundant re-reading makefiles.

[Miguel Ojeda]

On Thu, Jan 5, 2023 at 8:51 PM Alexander Pantyukhin
<apantykhin@gmail.com> wrote:
>
> I checked files now. It seems only one place in /samples  has 2 files
> with 1 makefile in the parent folder. So it looks like the patch is
> untimely for now.

Yeah, though you could do a benchmark with more files to account for
the future :)

> I see, thanks. Still have a question about using "with". Is it reasonable?

It is definitely reasonable and you cannot go wrong with `with`. I am
not sure if matters much in this case (short-lived script, read-only,
not a huge amount of files...), and with CPython the objects are
destroyed immediately anyway I believe.

Cheers,
Miguel