depmod: Minor performance improvements #158

stoeckmann · 2024-09-27T19:33:19Z

We can skip a calloc and turn it into a malloc because newly allocated memory is directly overwritten
The modules.order file can be interated just once
Minor cleanups (fixed typo and added a missing NULL check)

tools/depmod.c

Signed-off-by: Tobias Stoeckmann <[email protected]>

No need to clear newly allocated memory if source is copied into destination directly. Simplify code by using memdup from shared. Signed-off-by: Tobias Stoeckmann <[email protected]>

If memdup fails, handle out of memory condition. Signed-off-by: Tobias Stoeckmann <[email protected]>

evelikov

Apart from the minor suggestions it looks great. Thanks

evelikov · 2024-09-28T15:59:14Z

tools/depmod.c

-	unsigned idx = 0, total = 0;
+	unsigned idx = 0;
+	// all sorted modules shall have precedence
+	int i = -depmod->modules.count;


Can we make this unsigned total = depmod->modules.count; and keep the sort_idx = idx - total; below?

Humble poke ^^, or am I missing something?

For one, unsigned is the wrong data type. I have adjusted idx now to be size_t because depmod->modules.count is size_t as well and idx is used as iterator later on. Second, we know that we handle negative values here and sort_idx is an int, so there is no use in pretending that we have unsigned values at hand.

I have adjusted the code to verify that UINT16_MAX is not reached for depmod->modules.count, so it's verified that the int conversion fits into the data type (limit already exists but the check could be removed right now if asserts are disabled).

Regardless, I have added a cast here to explicitly show that I really know that depmod->modules.count will fit into an int without turning negative and that we are handling negative values here.

And last but not least: idx is used as a line counter, but we should handle duplicate lines, so if we go with idx - total, the result could become positive again.

evelikov · 2024-09-28T16:00:00Z

tools/depmod.c

 		mod = hash_find(depmod->modules_by_uncrelpath, line);
-		if (mod == NULL)
+		if (mod == NULL || mod->sort_idx < 0)


When can the sort index be negative? Should we split that into a separate patch?

It's negative if we encounter the line twice, i.e. someone entered the same line multiple times in input file. This could also allow to overflow any kind of counter.

In general, this value is supposed to be negative to be in front of all other indices written so far. So "unsigned int" makes a rather wrong impression on what's going on here.

It sounds like a pre existing bug, which ideally should be a separate patch.

I can strip it out of this commit, but now that you've mentioned it: I didn't notice that it was a bug before as well. It occurred to me because I was trying to think about how to break my new approach.

I'd prefer to keep it as it is, because otherwise we would have to keep track which line we have seen so far., bloating code which is removed with the next commit. So ... it's a nice bonus due to rewrite. ;)

Sorry, I don't follow - what tracking/bloat are you referring to... Having the below as prep commit should be enough, no?

- if (mod == NULL) + if (mod == NULL || mod->sort_idx < 0)

Oh yes, adding this AFTER my commit makes sense. I thought about adding this check to existing code, which would have been very complicated. Sorry for the noise. Adjusted.

The current code iterates through modules.order twice. First, it figures out how many lines exist. Then it iterates again to make sure that the first line has the lowest sort index, then ascending. Negative values make sure that the previously assigned positive values will be larger, i.e. modules in modules.order take precedence, then modules found in file system which were not listed in modules.order. This can be simplified by setting the initial sort index value to the lowest possible value needed, i.e. -depmod->modules.count. With this value, it is possible to iterate only once. Signed-off-by: Tobias Stoeckmann <[email protected]>

If the same line exists multiple times in modules.sort, consider only the first, since this is the earliest position requested. This also makes sure that index iterator never turns positive or would ever trigger a signed integer overflow. Signed-off-by: Tobias Stoeckmann <[email protected]>

The code does not support 65535 or more modules. Since this value is probably never reached, add a proper check after array building and do not rely solely on an assert later in code path. Signed-off-by: Tobias Stoeckmann <[email protected]>

Signed-off-by: Tobias Stoeckmann <[email protected]> Reviewed-by: Emil Velikov <[email protected]> Link: #158 Signed-off-by: Lucas De Marchi <[email protected]>

No need to clear newly allocated memory if source is copied into destination directly. Simplify code by using memdup from shared. Signed-off-by: Tobias Stoeckmann <[email protected]> Reviewed-by: Emil Velikov <[email protected]> Link: #158 Signed-off-by: Lucas De Marchi <[email protected]>

If memdup fails, handle out of memory condition. Signed-off-by: Tobias Stoeckmann <[email protected]> Reviewed-by: Emil Velikov <[email protected]> Link: #158 Signed-off-by: Lucas De Marchi <[email protected]>

The current code iterates through modules.order twice. First, it figures out how many lines exist. Then it iterates again to make sure that the first line has the lowest sort index, then ascending. Negative values make sure that the previously assigned positive values will be larger, i.e. modules in modules.order take precedence, then modules found in file system which were not listed in modules.order. This can be simplified by setting the initial sort index value to the lowest possible value needed, i.e. -depmod->modules.count. With this value, it is possible to iterate only once. Signed-off-by: Tobias Stoeckmann <[email protected]> Reviewed-by: Emil Velikov <[email protected]> Link: #158 Signed-off-by: Lucas De Marchi <[email protected]>

If the same line exists multiple times in modules.order, consider only the first, since this is the earliest position requested. This also makes sure that index iterator never turns positive or would ever trigger a signed integer overflow. Signed-off-by: Tobias Stoeckmann <[email protected]> Reviewed-by: Emil Velikov <[email protected]> Link: #158 Signed-off-by: Lucas De Marchi <[email protected]>

The code does not support 65535 or more modules. Since this value is probably never reached, add a proper check after array building and do not rely solely on an assert later in code path. Signed-off-by: Tobias Stoeckmann <[email protected]> Reviewed-by: Emil Velikov <[email protected]> Link: #158 Signed-off-by: Lucas De Marchi <[email protected]>

lucasdemarchi · 2024-10-01T14:13:51Z

I did a s/modules.sort/modules.order/ in one of the commit messages and applied. Thanks.

stoeckmann changed the title ~~depmod: Fix typo in comment~~ depmod: Minor performance improvements Sep 27, 2024

stoeckmann force-pushed the depmod_typo branch from 27d6fcf to 284ae1a Compare September 27, 2024 21:37

lucasdemarchi reviewed Sep 28, 2024

View reviewed changes

tools/depmod.c Show resolved Hide resolved

stoeckmann added 3 commits September 28, 2024 12:19

depmod: Fix typo in comment

4b4f7c5

Signed-off-by: Tobias Stoeckmann <[email protected]>

depmod: Use memdup

72ce75c

No need to clear newly allocated memory if source is copied into destination directly. Simplify code by using memdup from shared. Signed-off-by: Tobias Stoeckmann <[email protected]>

depmod: Check memdup return value

785e8f6

If memdup fails, handle out of memory condition. Signed-off-by: Tobias Stoeckmann <[email protected]>

stoeckmann force-pushed the depmod_typo branch from 284ae1a to fff839b Compare September 28, 2024 10:21

evelikov requested changes Sep 28, 2024

View reviewed changes

stoeckmann force-pushed the depmod_typo branch 2 times, most recently from a798387 to 33bb2cc Compare September 30, 2024 15:06

stoeckmann added 3 commits September 30, 2024 17:10

depmod: Check amount of memories

a431787

The code does not support 65535 or more modules. Since this value is probably never reached, add a proper check after array building and do not rely solely on an assert later in code path. Signed-off-by: Tobias Stoeckmann <[email protected]>

stoeckmann force-pushed the depmod_typo branch from 33bb2cc to a431787 Compare September 30, 2024 15:10

lucasdemarchi pushed a commit that referenced this pull request Oct 1, 2024

depmod: Fix typo in comment

806c304

Signed-off-by: Tobias Stoeckmann <[email protected]> Reviewed-by: Emil Velikov <[email protected]> Link: #158 Signed-off-by: Lucas De Marchi <[email protected]>

lucasdemarchi closed this Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

depmod: Minor performance improvements #158

depmod: Minor performance improvements #158

stoeckmann commented Sep 27, 2024 •

edited

Loading

evelikov left a comment

evelikov Sep 28, 2024

evelikov Sep 30, 2024

stoeckmann Sep 30, 2024

evelikov Sep 28, 2024

stoeckmann Sep 28, 2024

evelikov Sep 29, 2024

stoeckmann Sep 29, 2024 •

edited

Loading

stoeckmann Sep 29, 2024

evelikov Sep 29, 2024

stoeckmann Sep 29, 2024

lucasdemarchi commented Oct 1, 2024

depmod: Minor performance improvements #158

depmod: Minor performance improvements #158

Conversation

stoeckmann commented Sep 27, 2024 • edited Loading

evelikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stoeckmann Sep 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasdemarchi commented Oct 1, 2024

stoeckmann commented Sep 27, 2024 •

edited

Loading

stoeckmann Sep 29, 2024 •

edited

Loading