Write the detect cycles function as a function to optimize code #1013

wdfk-prog · 2024-08-01T09:39:07Z

Using the master branch code, the footprint size of the gcc compilation under Windows is as follows

   text    data     bss     dec     hex filename
  33440       0       0   33440    82a0 lfs.o
    208       0       0     208      d0 lfs_util.o
  33648       0       0   33648    8370 (TOTALS)

After this change, the compilation footprint is as follows

   text    data     bss     dec     hex filename
  33228       0       0   33228    81cc lfs.o
    208       0       0     208      d0 lfs_util.o
  33436       0       0   33436    829c (TOTALS)

The optimization result can be obtained,Compiles without error or warning.

  0x8370- 0x829c = 0xD4
  0xD4/0x8370 = 0.63%

gcc version

Using built-in specs.
COLLECT_GCC=C:\MinGW\bin\gcc.exe
COLLECT_LTO_WRAPPER=c:/mingw/bin/../libexec/gcc/mingw32/9.2.0/lto-wrapper.exe
Target: mingw32
Configured with: ../src/gcc-9.2.0/configure --build=x86_64-pc-linux-gnu --host=mingw32 --target=mingw32 --disable-win32-registry --with-arch=i586 --with-tune=generic --enable-static --enable-shared --enable-threads --enable-languages=c,c++,objc,obj-c++,fortran,ada --with-dwarf2 --disable-sjlj-exceptions --enable-version-specific-runtime-libs --enable-libgomp --disable-libvtv --with-libiconv-prefix=/mingw --with-libintl-prefix=/mingw --enable-libstdcxx-debug --disable-build-format-warnings --prefix=/mingw --with-gmp=/mingw --with-mpfr=/mingw --with-mpc=/mingw --with-isl=/mingw --enable-nls --with-pkgversion='MinGW.org GCC Build-2'
Thread model: win32
gcc version 9.2.0 (MinGW.org GCC Build-2)

wdfk-prog · 2024-08-01T09:43:48Z

Also worth mentioning is the use of gcc and makefiles on Windows; Can only compile and clear, view the overall occupancy size and generate assembly files, other operations can not take effect, will give an error; Is there any plan to support this part on Windows.

wdfk-prog · 2024-08-02T01:12:08Z

CI errors are all Error: The operation was canceled.

BenBE

The minor style stuff aside, I think there's a slight problem with that algorithm as it changes behaviour by moving the initialization part into the loop. This might still find trivial cases of loops, but will hide loops, that need the iteration behaviour of advancing the tortoises. You can check this by noticing that several of the assignments of the function lfs_detect_cycles are now dead stores, when previously they would be re-used in the next loop iteration. Nonetheless, centralizing this check pattern may allow code size reduction.

To fix that bug, the following should be done:

Introduce a structure to hold the state of the algorithm

typedef struct lfs_tortoise {
    lfs_block_t block[2]; // Previously tortoise
    lfs_size_t i; // Previously tortoise_i
    lfs_size_t period; // Previously tortoise_period
} lfs_tortoise_t;

Pass a lfs_tortoise_t* as the second argument to lfs_detect_cycles
On the call sites: Outside the loop initialize a lfs_tortoise_t like so:

lfs_tortoise_t tortoise = {
    .block = {LFS_BLOCK_NULL, LFS_BLOCK_NULL},
    .i = 1,
    .period = 1,
};

Pass that variable to lfs_detect_cycles by reference.

The "cancelled" CI jobs were likely due to some loop in the filesystem under test not being detected after your change.

lfs.c

wdfk-prog · 2024-08-03T01:05:12Z

The minor style stuff aside, I think there's a slight problem with that algorithm as it changes behaviour by moving the initialization part into the loop. This might still find trivial cases of loops, but will hide loops, that need the iteration behaviour of advancing the tortoises. You can check this by noticing that several of the assignments of the function lfs_detect_cycles are now dead stores, when previously they would be re-used in the next loop iteration. Nonetheless, centralizing this check pattern may allow code size reduction.撇开次要风格的东西不谈，我认为该算法存在一个小问题，因为它通过将初始化部分移动到循环中来改变行为。这仍然可能找到循环的微不足道的情况，但会隐藏循环，这些循环需要推进的迭代行为。您可以通过注意到函数lfs_detect_cycles的几个赋值现在是死存储来检查这一点，而以前它们将在下一次循环迭代中重用。尽管如此，集中管理此检查模式可能会减小代码大小。

To fix that bug, the following should be done:要修复该错误，应执行以下操作：

Introduce a structure to hold the state of the algorithm引入一个结构来保存算法的状态
typedef struct lfs_tortoise {
    lfs_block_t block[2]; // Previously tortoise
    lfs_size_t i; // Previously tortoise_i
    lfs_size_t period; // Previously tortoise_period
} lfs_tortoise_t;
Pass a lfs_tortoise_t* as the second argument to 将 lfs_tortoise_t* 作为第二个参数传递给lfs_detect_cycles

On the call sites: Outside the loop initialize a lfs_tortoise_t like so:在调用站点上：在循环外初始化一个lfs_tortoise_t，如下所示：
lfs_tortoise_t tortoise = {
    .block = {LFS_BLOCK_NULL, LFS_BLOCK_NULL},
    .i = 1,
    .period = 1,
};
Pass that variable to lfs_detect_cycles by reference.通过引用将该变量传递给 lfs_detect_cycles。

The "cancelled" CI jobs were likely due to some loop in the filesystem under test not being detected after your change.“已取消”的 CI 作业可能是由于在更改后未检测到所测试文件系统中的某些循环。

The minor style stuff aside, I think there's a slight problem with that algorithm as it changes behaviour by moving the initialization part into the loop. This might still find trivial cases of loops, but will hide loops, that need the iteration behaviour of advancing the tortoises. You can check this by noticing that several of the assignments of the function lfs_detect_cycles are now dead stores, when previously they would be re-used in the next loop iteration. Nonetheless, centralizing this check pattern may allow code size reduction.

To fix that bug, the following should be done:

Introduce a structure to hold the state of the algorithm
typedef struct lfs_tortoise {
    lfs_block_t block[2]; // Previously tortoise
    lfs_size_t i; // Previously tortoise_i
    lfs_size_t period; // Previously tortoise_period
} lfs_tortoise_t;
Pass a lfs_tortoise_t* as the second argument to lfs_detect_cycles

On the call sites: Outside the loop initialize a lfs_tortoise_t like so:
lfs_tortoise_t tortoise = {
    .block = {LFS_BLOCK_NULL, LFS_BLOCK_NULL},
    .i = 1,
    .period = 1,
};
Pass that variable to lfs_detect_cycles by reference.

The "cancelled" CI jobs were likely due to some loop in the filesystem under test not being detected after your change.

Thank you for reminding me that I made a stupid mistake

BenBE · 2024-08-03T17:52:03Z

lfs.c

+ struct lfs_tortoise_t tortoise = {
+ .pair = {LFS_BLOCK_NULL, LFS_BLOCK_NULL},
+ .i = 1,
+ .period = 1,
+ };


This needs to go outside the loop. Similar for the other 3 places.

o(╥﹏╥)o,Sorry, I was so careless.

geky-bot · 2024-08-04T02:20:14Z

Tests passed ✓, Code: 17096 B (+0.2%), Stack: 1448 B (+0.6%), Structs: 812 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	17096 B (+0.2%)	1448 B (+0.6%)	812 B (+0.0%)	Lines	2373/2548 lines (+0.1%)
Readonly	6190 B (-0.1%)	448 B (+0.0%)	812 B (+0.0%)	Branches	1241/1580 branches (-0.1%)
Threadsafe	17956 B (+0.2%)	1448 B (+0.6%)	820 B (+0.0%)		Benchmarks
Multiversion	17164 B (+0.2%)	1448 B (+0.6%)	816 B (+0.0%)	Readed	29369693876 B (+0.0%)
Migrate	18788 B (+0.1%)	1744 B (+0.0%)	816 B (+0.0%)	Proged	1482874766 B (+0.0%)
Error-asserts	17780 B (+0.2%)	1440 B (+0.6%)	812 B (+0.0%)	Erased	1568888832 B (+0.0%)

geky · 2024-09-20T06:38:53Z

lfs.c

+ lfs_size_t period;
+};
+
+static int lfs_detect_cycles(lfs_mdir_t dir, struct lfs_tortoise_t *tortoise)


It looks like you're passing the dir by value, instead of by pointer. This creates a copy of the 8-word struct, which might be adding both stack cost and code cost.

Passing by pointer:

static int lfs_detect_cycles(const lfs_mdir_t *dir, struct lfs_tortoise_t *tortoise) {

I missed it, it really shouldn't be, ^_^.
I've resubmitted it.

geky · 2024-09-20T06:52:40Z

Also worth mentioning is the use of gcc and makefiles on Windows; Can only compile and clear, view the overall occupancy size and generate assembly files, other operations can not take effect, will give an error; Is there any plan to support this part on Windows.

Unfortunately Windows is a difficult platform to support as a development environment. It's just not worth supporting vs leveraging all of the useful tools that are available in Linux.

The best option would be use either a virtual machine or WSL to run a Linux image on windows and develop inside that. I've heard good things about WSL but haven't used it myself.

geky · 2024-09-24T17:30:33Z

Sorry, this failure was an issue on our side. GitHub made a breaking change to their APIs recently.

To fix the pr this needs to be rebased onto the devel branch: https:/littlefs-project/littlefs/tree/devel, let me know if you want me to do that for you:

$ git rebase origin/devel

wdfk-prog · 2024-09-25T00:14:02Z

Sorry, this failure was an issue on our side. GitHub made a breaking change to their APIs recently.很抱歉，此失败是我们这边的问题。GitHub 最近对其 API 进行了重大更改。

To fix the pr this needs to be rebased onto the devel branch: https:/littlefs-project/littlefs/tree/devel, let me know if you want me to do that for you:要修复 pr，这需要重新基到 devel 分支上：https:/littlefs-project/littlefs/tree/devel，如果您希望我为您执行此操作，请告诉我：
$ git rebase origin/devel

OK

geky · 2024-10-02T15:21:19Z

Sorry again, I was having problems pushing to your branch and couldn't understand why. You set "Maintainers are allowed to edit this pull request." correctly but pushes are still being rejected for some reason.

And wow, I think this is a real mess up on GitHub's part. There is apparently a bug where cross-organization forks can't be edited by maintainers, and I think this might also be affecting forks-of-cross-org-forks:
https:/orgs/community/discussions/5634

So unfortunately it looks like you either need to add me as a collaborator or do the rebase locally... Fortunately it's a simple rebase with no conflicts.

wdfk-prog · 2024-10-03T01:36:25Z

Sorry again, I was having problems pushing to your branch and couldn't understand why. You set "Maintainers are allowed to edit this pull request." correctly but pushes are still being rejected for some reason.

And wow, I think this is a real mess up on GitHub's part. There is apparently a bug where cross-organization forks can't be edited by maintainers, and I think this might also be affecting forks-of-cross-org-forks: https:/orgs/community/discussions/5634

So unfortunately it looks like you either need to add me as a collaborator or do the rebase locally... Fortunately it's a simple rebase with no conflicts.

I don't know if my operation is correct, you can check it.

geky-bot · 2024-10-03T03:41:27Z

Tests passed ✓, Code: 17000 B (-0.9%), Stack: 1440 B (+0.0%), Structs: 812 B (+0.0%)

	Code	Stack	Structs		Coverage
Default	17000 B (-0.4%)	1440 B (+0.0%)	812 B (+0.0%)	Lines	2373/2548 lines
Readonly	6198 B (+0.1%)	448 B (+0.0%)	812 B (+0.0%)	Branches	1241/1580 branches
Threadsafe	17860 B (-0.4%)	1440 B (+0.0%)	820 B (+0.0%)		Benchmarks
Multiversion	17072 B (-0.3%)	1440 B (+0.0%)	816 B (+0.0%)	Readed	29369693876 B
Migrate	18688 B (-0.4%)	1736 B (-0.5%)	816 B (+0.0%)	Proged	1482874766 B
Error-asserts	17680 B (-0.4%)	1432 B (+0.0%)	812 B (+0.0%)	Erased	1568888832 B

geky · 2024-10-03T18:56:04Z

Looks good here, thanks for that. I've manually updated the bot comment since it's still a bit broken on our end.

lfs.c

geky · 2024-10-03T19:16:29Z

I left some review comments, mostly related to style/consistency with the rest of the codebase.

But this looks good to me and we can probably bring it in on the next minor release.

wdfk-prog · 2024-10-04T02:53:35Z

I left some review comments, mostly related to style/consistency with the rest of the codebase.

But this looks good to me and we can probably bring it in on the next minor release.

I have made the corresponding modification, maybe it should be marked with some normative information in the pr content;
You can refer to the link below:https:/RT-Thread/rt-thread/tree/master/.github
It is recommended to use a formatted script before submitting, such as the following link:https:/mysterywolf/formatting

geky-bot · 2024-10-04T03:02:16Z

Tests passed ✓, Code: 17004 B, Stack: 1440 B, Structs: 812 B

	Code	Stack	Structs		Coverage
Default	17004 B	1440 B	812 B	Lines	2378/2553 lines
Readonly	6202 B	448 B	812 B	Branches	1241/1580 branches
Threadsafe	17864 B	1440 B	820 B		Benchmarks
Multiversion	17076 B	1440 B	816 B	Readed	29369693876 B
Migrate	18692 B	1736 B	816 B	Proged	1482874766 B
Error-asserts	17684 B	1432 B	812 B	Erased	1568888832 B

geky · 2024-10-04T19:30:14Z

Thanks for making those changes. This looks good to me and will bring it in next release.

maybe it should be marked with some normative information in the pr content;

Point taken, it is a waste of both your time and my time commenting on style inconsistencies.

It is recommended to use a formatted script before submitting, such as the following link:https:/mysterywolf/formatting

I had looked into astyle before, but struggled to make it do what I wanted. It was very opinionated about unnecessary things.

But a custom python script is a good idea, maybe that's the way to go.

wdfk-prog changed the title ~~Write the detect cycles function as a function to optimize your code~~ Write the detect cycles function as a function to optimize code Aug 1, 2024

BenBE suggested changes Aug 2, 2024

View reviewed changes

lfs.c Outdated Show resolved Hide resolved

lfs.c Outdated Show resolved Hide resolved

lfs.c Outdated Show resolved Hide resolved

lfs.c Outdated Show resolved Hide resolved

wdfk-prog force-pushed the feature_2.9.3 branch from dddbe2f to 7cb6a79 Compare August 3, 2024 01:18

BenBE reviewed Aug 3, 2024

View reviewed changes

wdfk-prog force-pushed the feature_2.9.3 branch from 7cb6a79 to 0e8f4d7 Compare August 4, 2024 01:57

geky added the code size label Sep 20, 2024

geky reviewed Sep 20, 2024

View reviewed changes

wdfk-prog force-pushed the feature_2.9.3 branch from 0e8f4d7 to ea53106 Compare September 22, 2024 12:59

geky changed the base branch from master to devel September 24, 2024 17:27