Tiny Core Linux
Tiny Core Base => TCB Talk => Topic started by: alphons on February 14, 2023, 09:19:08 AM
-
Hello All,
I submitted a patch for /usr/sbin/rebuildfstab at github. And then it started to get weird.
--
The current rebuildfstab script is flawed having a big penalty in user, but more over system time.
The script is run every time a block device is added or removed (udev rules)
I made some changes to get it right, removed the bad loops in the script.
Ok, so far so good. These are some results on a system having some disks and more than 200 loop devices.
----
Old rebuildfstab
real 0m 0.47s
user 0m 0.08s
sys 0m 0.38s
New rebuildfstab
real 0m 0.14s
user 0m 0.03s
sys 0m 0.08s
----
These are execellent results.
When i made a pull request things got out of hand, to my opinion.
--clbr--
Thank you for the patch. While your numbers look good, it would change the behavior, and possibly have speed regressions in some envs. The current code supports manual fstab entries, while this patch would add those a second time. Running blkid without any options does a slow scan, which with optical drives and floppies can be slow (I'm not sure if the -s option also triggers this - it's possible that also does a scan).
Perhaps a large part of the time taken is by the "find" command, where adding "-maxdepth 2" may give some speedup.
If you'd like to submit the ext4 change separately, please do.
--clbr--
Oh, about the header edits: removing the includes also may change behavior, and the header comment seems improper when no other contributors have been added therein.
---------
So this is a big NO-GO to opinion o *clbr*
These are my remarks on the so called 'code review'
- "it would change the behavior" NO
- "The current code supports manual fstab entries, while this patch would add those a second time" Also NO, this update does exactly what the other code does, only better and faster
- "blkid without any options does a slow scan", NO, have a look at the source code.
- "with optical drives and floppies can be slow", sure also the NFS mounts can be slow, this has nothing to do with 99% of us
- "The current code supports manual fstab entries, while this patch would add those a second time" NO. The behaviour of this code is exactly the same as the original code, have a look at that source, and come back with better comments
- "If you'd like to submit the ext4 change separately, please do." NO, it is all or Nothing, but I get the picture already.
To bad, this code review does not have the high quality i want.
- "removing the includes also may change behavior" NO, no functions are used from the header includes.
- "header comment seems improper when no other contributors have been added therein" NO, I want some more comment, what the script does exactly and so on, this is a start
--clbr--
Very well, closing this. "come back with better comments" when your patch is wrong is simply rude.
--------
Is this collaboration??? I am not on your payrol mister.
Prove us where the script is wrong and can be made better and faster.
Cheers.
ps, Attached my dmesg of a test system having the patch, running /init after booting on 0.6 sec (!!!) mounting swap and data disk and nic up. All under a total of 1.7 seconds (!!!)
-
Without installed any extensions (no loop devices), the timing of my new rebuildfstab goes down to 40mS.....
Screenshot attached, including free, df and /etc/fstab
But, nobody cares.
-
u havent even linked the patch here, how am i supposed to judge now
-
What hiro said.
P.S. TCL is overseen by a small number of developers. They are extremely conservative. In an age where software bloat and complexity is spreading like cancer, I think the temperament of the caretakers is an asset. I've been using TCL for several years and have noticed that the developers prioritize simplicity and compatibility with old hardware (e.g., optical drives) over performance. I haven't seen your patch but if it adds complexity or the potential to break compatibility with old hardware, I'm not surprised that you encountered some resistance.
-
I do not do programming, but for who ever is interested in the good/bad patch discussion/improvement and justify it why, please find the original link here:
https://github.com/tinycorelinux/Core-scripts/pull/39/commits/abc8b009b7ac25810aae49d7b3ec4652fbcb0d6b (https://github.com/tinycorelinux/Core-scripts/pull/39/commits/abc8b009b7ac25810aae49d7b3ec4652fbcb0d6b)
From my lay-man point of view:1. in usr/sbin/rebuildfstab
- wrong eliminated busybox compatibility sintax (useBusybox ) ?
- no need to add copyright for every pi-nuts
- eliminated CDROM cases: case "$CDROMS" in *"$DEVROOT/$DEVNAME"*) FSTYPE="auto" ;; esac
but some good tthings like:
- DEVMAJOR="$(cat $BLOCKDEV|cut -f1 -d:)"
+ DEVMAJOR="$(cat /sys/class/block/$DEVNAME/dev |cut -f1 -d:)"
- ext2|ext3) OPTIONS="${OPTIONS},relatime" ;;
+ ext2|ext3|ext4) OPTIONS="${OPTIONS},relatime" ;;
- mkdir -p "/mnt/$DEVNAME" 2>/dev/null >/dev/null
+ mkdir -p "/mnt/$DEVNAME" >/dev/null 2>&1
so the main thing time-consuming remains awk loop.
-
It seems the issue is only partly technical. Main issue seems to be social/interpersonal. I'm sorry to have gotten involved.
-
both sides are rude here. but there seems to be a language barrier, too.
i'm sure we can make something out of your idea, whether with this patch or another one later.
without testing anything, just from looking at your patch, i guess the main performance bottleneck you found is that blkid is run multiple times?
-
for reference, this is what blkid's -s tag does (from the man page):
For each (specified) device, show only the tags that match tag. It is possible to specify multiple -s options. If no tag is specified, then all tokens are shown for all (specified) devices. In order to just refresh the cache without showing any tokens, use -s none with no other options.
-
- "blkid without any options does a slow scan", NO, have a look at the source code.
sure you aren't being actively misleading here?
as said in the last message, i can't imagine any other reason apart from blkid's slowness in the first place.
-
Alphons does not understand what the original code did, mindlessly removes it, then insists he is correct without understanding the issue, and insists on visible credit when his contribution is much smaller than previous contributions by aswhj etc, and when in the git era the credit info is kept in git commits.
"Also NO, this update does exactly what the other code does, only better and faster"
Let me spell out how it changes behavior and breaks the custom mounts. Say you have a custom fstab line mounting /dev/sda1 to /files. After the patch, it would add /dev/sda1 a second time, breaking the custom mountpoint.
" NO, no functions are used from the header includes."
Wrong. He didn't even read the code he changed, and then insists he is right. useBusybox is the function called, which affects among others cat, grep, sort, sync, etc. called in the script.
-
he seemed to try to fish for credit, but i gave it a pass bec. he might have seen the pattern of adding the authors in another file that he also had sent a patch for.
but i also found an older patch of them about colors which had to be reverted bec. it also unintendedly broke other things.
a lot of momentum, but sadly it's a wasted effort here...
you'll have to move fast and break things on another project.
-
I have followed this conversation with interest. I'm not going to judge who is wrong or right here (that is not my place), but as a TCL enthusiast, who wants to learn how TCL exactly works, I can share that it is hard to understand what the standard scripts do that ship with the release.
There are almost no comments in the code and there is no header section or alike, with an overview of what the intent of the script is. So you always have to read the code and reverse engineer. Then it is easy to overlook use cases or nuances if you change the code.
-
So you always have to read the code.
welcome to the real world
-
welcome to the real world
In the real world, writing and maintaining comments is considered a good thing. If you mean that despite this, in the real world useful comments are often missing so you have to reverse engineer the code anyway, then I agree.
-
Hi FlyingDutchman
... useful comments are often missing so you have to reverse engineer the code ...
I've run into that issue too:
Hi Juanito
I'd almost forgotten about that - thanks :)
I rediscovered this thread about 6 weeks ago and left an open tab to it in my browser. That solved the forgetting part, but
not the procrastination because I found it difficult to follow and understand the code. You can't fix what you don't understand. ::)
Had I reformatted and commented the code from the get go, I would have been done a lot sooner. :-[ ...
-
I thought that comments were another thing where there was a balance with keeping things tiny. More text equals larger script files, which overall might be significant. A very well commented script might be twice the size of a sparsely commented one, or more, and uncomressed text isn't all the small, so I assume the TC developers want to strike a balance.
Anyway when this was first posted I took a look and was able to spot some optimisations in the script that were possible with much less significant changes to how the script operates.
# echo modified: ;time rebuildfstab-mod; echo original: ; time rebuildfstab
modified:
real 0m 0.40s
user 0m 0.07s
sys 0m 0.21s
original:
real 0m 0.63s
user 0m 0.23s
sys 0m 0.38s
But I haven't got around to testing that it really works the same in lots of different circumstances yet.