Get_next_datachange: use start to compute timeout

[tobast]

Related to issue #1102. Tested hastily, seems to fix the high CPU load.

[tobast]

One test does not pass: test_datachange_ttl. The reason is that this test uses a timestamp that is not time_monotonic_now for simplicity, but queues.c:506 calls time_monotonic_now, and messes up n->start wrt. the timestamp as seen by the test.

[tobast]

One test does not pass: test_datachange_ttl. The reason is that this test uses a timestamp that is not time_monotonic_now for simplicity, but queues.c:506 calls time_monotonic_now, and messes up n->start wrt. the timestamp as seen by the test.

Update: it seems fine to use the time variable at those locations, so I changed it. The test suite now fully passes.

[fwsmit]

It still fails the the test_datachange_agethreshold_at_second test

[tobast]

Indeed, I was too hasty. I'm checking why.

EDIT: Ah, this is because c25d916 changed slightly the semantics of next_datachange. I probably did not re-run the testsuite after this one. I'm fixing the test.

[tobast]

@fwsmit The test is fixed in the latest revision

[codecov-commenter]

Codecov Report

Merging #1158 (15b0941) into master (464076d) will increase coverage by 0.01%.
The diff coverage is 84.61%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #1158      +/-   ##
==========================================
+ Coverage   66.03%   66.04%   +0.01%     
==========================================
  Files          46       46              
  Lines        7595     7601       +6     
==========================================
+ Hits         5015     5020       +5     
- Misses       2580     2581       +1     

Flag	Coverage Δ
unittests	66.04% <84.61%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/notification.c	60.47% <0.00%> (-0.15%)	⬇️
src/queues.c	92.06% <100.00%> (+0.05%)	⬆️
test/queues.c	98.97% <100.00%> (+<0.01%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[fwsmit]

I think you should subtract here. It should seem as if the notification has been sent TURN_OF_SECOND_THRESHOLD_US earlier, so that the turn of second is a little earlier (please correct me if I'm wrong)

[tobast]

Actually, this is just the wrong way to do it. It is time_monotonic_now() that should be adjusted, not the n->timestamp (though both work of course, but it is way cleaner this way).

[tobast]

Sorry, I notice I forgot to say that the last revision should have fixed this.

[fwsmit]

With the understanding of #1102 (comment), I want to suggest to split this PR up in 2 parts. One part being fixing the mixup between n->start and n->timestamp. The other part being the feature to be a bit more lenient in the required sleep duration.
I want to release the first part in the next point release to fix the cpu usage issues and the other part can wait till a next version.

Are you willing to split up this PR? It should be as easy as submitting the right commits

[tobast]

I just checked, and it seems to be totally possible. The first two commits (5525e99, cd65209) should go together in the point release, as the second one ensures that the tests are still passing; the third one (15b0941) can go in later (this is the one that tries to be clever about turns of second).

I'll split the PR very soon.

[tobast]

This third commit is dropped from this PR in the current revision. The new PR #1167 covers it.

[fwsmit]

Thanks, I'm merging this now. It would be good to add a few tests to prevent issues like this and similar in the future, but I'll be making that a separate issue