Don't use inefficient arrays to queue signal in maintain_log

[rany2]

CC @lynxthecat

[rany2]

@lynxthecat can you test if this negatively impacts performance on your router compared to current master?

[lynxthecat]

OK I'll test. But any loss with using arrays on signal interception (exception rather than the norm) is surely outweighed by the loss with using three tests without any signals (norm rather than the exception)?

[rany2]

@lynxthecat it's just to get an idea, i'll have it use switch-cases in a bit

[lynxthecat]

It does make me wonder if such logic could be used elsewhere like in some of the calculations.

[lynxthecat]

It looks like the three tests hurt quite a bit:

root@OpenWrt-1:~/cake-autorate# ./bench_cpu.sh
Running CPU benchmark for test period of: 60s.
...
Average CPU usage over test period of 60s was: 6.679%

root@OpenWrt-1:~/cake-autorate# cp /tmp/cake-autorate/cake-autorate.sh .
root@OpenWrt-1:~/cake-autorate# ./bench_cpu.sh
Running CPU benchmark for test period of: 60s.
...
Average CPU usage over test period of 60s was: 7.632%

[rany2]

@lynxthecat test now please

[lynxthecat]

OK, but isn't it getting more and more ugly!

[rany2]

@lynxthecat thanks!

[lynxthecat]

root@OpenWrt-1:~/cake-autorate# ./bench_cpu.sh
Running CPU benchmark for test period of: 60s.
...
Average CPU usage over test period of 60s was: 6.553%

[rany2]

@lynxthecat did you use the variant that logs out "unknown signal 0"?

[lynxthecat]

That still has to evaluate 3x operations though doesn't it? Or does the string comparison amount to the same anyway?

In any case, it does make me wonder if such logic could be leveraged elsewhere.

[lynxthecat]

@lynxthecat did you use the variant that logs out "unknown signal 0"?

How do you mean?

[lynxthecat]

Oh I see, I'll retest.

[lynxthecat]

root@OpenWrt-1:~/cake-autorate# cp /tmp/cake-autorate/cake-autorate.sh .
root@OpenWrt-1:~/cake-autorate# vim cake-autorate.sh
root@OpenWrt-1:~/cake-autorate# ./bench_cpu.sh
Running CPU benchmark for test period of: 60s.
...
Average CPU usage over test period of 60s was: 6.649

[rany2]

What's the baseline from master?

[lynxthecat]

Doing 2x head-to-head comparison. Two seconds.

[lynxthecat]

Here are 2x runs from this pull request, then 2x runs from master:

root@OpenWrt-1:~/cake-autorate# ./bench_cpu.sh
Running CPU benchmark for test period of: 60s.
...
Average CPU usage over test period of 60s was: 6.649%
root@OpenWrt-1:~/cake-autorate# ./bench_cpu.sh
Running CPU benchmark for test period of: 60s.
...
Average CPU usage over test period of 60s was: 6.629%
root@OpenWrt-1:~/cake-autorate# cp /tmp/cake-autorate/cake-autorate.sh .
root@OpenWrt-1:~/cake-autorate# ./bench_cpu.sh
Running CPU benchmark for test period of: 60s.
...
Average CPU usage over test period of 60s was: 6.607%
root@OpenWrt-1:~/cake-autorate# ./bench_cpu.sh
Running CPU benchmark for test period of: 60s.
...
Average CPU usage over test period of 60s was: 6.602%

[rany2]

I have another idea, just give me a moment @lynxthecat

[rany2]

@lynxthecat please try the current solution

[lynxthecat]

I will test once I return from walking our dog!

[lynxthecat]

Why not just put the printf calls inside the traps?

[rany2]

@lynxthecat because that would just be too ugly!

[lynxthecat]

Hmm! My array solution still seems preferable on the elegance score.

So we are converting to hex with function call to provide string comparison with hex?

[rany2]

Hmm! My array solution still seems preferable on the elegance score.

I don't like your solution because it means an impatient user could spam cake-autorate with say an export request and cake-autorate will just mindlessly keep queueing those requests. This prevents this situation without any additional checks while allowing users to make, for example, both export and reset requests. Also no dynamic array sizes.

So we are converting to hex with function call to provide string comparison with hex?

Yes, so we don't have to just match all the possible cases like we did before. I'd prefer to do signal & (1<<1) but bash is slow and to be able to use a switch-case, I'd need to match all the possible cases with 1<<1 being set. So, to match 1<<1 I'd need to match 1<<1|1<<2 and 1<<1 (assuming 1<<2 is next to be cleared in the switch-case order)

[lynxthecat]

There is surely an easier way. How about only adding array element if unique.

"${array[@]/$element/}" != "${array[@]}"

Also I’m not sure the export spam thing is so bad. I mean we’re honouring the requests sent by the user.

[rany2]

Also I’m not sure the export spam thing is so bad. I mean we’re honouring the requests sent by the user.

It is, because you also are waiting 0.5 seconds between every time you're processing it. So it could easily mean that the user will need to restart cake-autorate.

There is surely an easier way. How about only adding array element if unique.

"${array[@]/$element/}" != "${array[@]}"

I just felt like this solution was more efficient

[lynxthecat]

It is, because you also are waiting 0.5 seconds between every time you're processing it. So it could easily mean that the user will need to restart cake-autorate.

How do you mean? I mean what’s the significance of the timeout. And that’ll only be if read doesn’t return sooner.

[rany2]

How do you mean? I mean what’s the significance of the timeout. And that’ll only be if read doesn’t return sooner.

It makes the situation of queueing up these requests with your solution so much worse.

[rany2]

Anyway how does this perform on your router, I'm sure it's more efficient than the current implementation but by how much?

[lynxthecat]

root@OpenWrt-1:~/cake-autorate# ./bench_cpu.sh
Running CPU benchmark for test period of: 60s.
...
Average CPU usage over test period of 60s was: 6.607%
root@OpenWrt-1:~/cake-autorate# ./bench_cpu.sh
Running CPU benchmark for test period of: 60s.
...
Average CPU usage over test period of 60s was: 6.578%

[lynxthecat]

Same speedup can be given to master by just checking for an empty string:

root@OpenWrt-1:~/cake-autorate# ./bench_cpu.sh
Running CPU benchmark for test period of: 60s.
...
Average CPU usage over test period of 60s was: 6.599%

[rany2]

Closing as the solution was too unreadable.

[lynxthecat]

For the record, I admire the tenacity reflected in this approach. I am a huge fan of tenacity; it is surely a powerful force to be reckoned with.

[rany2]

Thanks @lynxthecat, though I was pretty disappointed in myself when I saw how simple your solution was. I have no idea why my thinking was so skewed towards this, if this were in C the initial variant of this PR would have made a ton of sense but it just kept getting more absurd until we reached this final bit with converting the thing to hex... :rofl:

So that was tragic !

[rany2]

until we reached this final bit with converting the thing to hex...

Oh, and how could I forget the masterpiece of just having a case contain all the possible combinations for a signal when I was just setting one bit at a time X)