GNOME Bugzilla – Bug 728163
[review] th/nm-online-timeout
Last modified: 2014-06-24 19:06:45 UTC
Please review the branch th/nm-online-timeout which changes the timeout calculation in nm-online. This calculates the timeout of nm-online more precisely and makes the progress bar printed more fluently. Also, allow the timeout to be specified with sub second precision.
(In reply to comment #0) > Also, allow the timeout to be specified with sub second precision. why?
(In reply to comment #1) > (In reply to comment #0) > > Also, allow the timeout to be specified with sub second precision. > > why? What the branch changes in first place is to be more exact in calculating the timeout. Without it, it is common to see it off by one second, sometimes significantly more. If you say -t 30 it should really take 30 seconds in a good approximation. Allowing sub second precision adds little complexity on top of that, so why restrict it?
There is no good use case for specifying sub-second precision. Just drop that part of it. All of the math around G_USEC_PER_SEC seems wrong. Eg: >+ /* schedule next timeout in @t usec. Setting the lower limit of 2 msec to avoid >+ * unneeded wakeups towards the end. */ >+ t = MAX (remaining_us, G_USEC_PER_SEC/200); G_USEC_PER_SEC/200 is 5000, aka 5 msec. It seems like everything would be simpler if you just did all the math in milliseconds rather than microseconds, since that's what g_timeout_add() wants anyway.
(In reply to comment #3) Pushed fixup commits > There is no good use case for specifying sub-second precision. Just drop that > part of it. I don't see the gain to restrict something that comes for free. Anyway. fixup pushed. > All of the math around G_USEC_PER_SEC seems wrong. Eg: (?) I didn't see any error, except... > > >+ /* schedule next timeout in @t usec. Setting the lower limit of 2 msec to avoid > >+ * unneeded wakeups towards the end. */ > >+ t = MAX (remaining_us, G_USEC_PER_SEC/200); The code comment was wrong. Anyway, I removed this line entirely. > G_USEC_PER_SEC/200 is 5000, aka 5 msec. > > It seems like everything would be simpler if you just did all the math in > milliseconds rather than microseconds, since that's what g_timeout_add() wants > anyway. Done.
(Beware that two of your fixups reference the wrong commit.) >+ if (((percentage * PROGRESS_STEPS) - progress_next_step_i) > 0.0) >+ progress_next_step_i++; /* ceil() */ ... >+ gint64 progress_step_duration = (timeout->end_timestamp_ms - timeout->start_timestamp_ms + PROGRESS_STEPS/2) / PROGRESS_STEPS; Two different rounding behaviors. >+ double percentage = ((double) (now - timeout->start_timestamp_ms)) / (timeout->end_timestamp_ms - timeout->start_timestamp_ms); We don't really need floating point in this code... Just use integer math on the millisecond values; the user will not notice sub-millisecond rounding errors. Also, if you computed progress_step_duration before the loop started, then progress_next_step_i would be just (now - timeout->start_timestamp_ms) / timeout->progress_step_duration;
(In reply to comment #5) > (Beware that two of your fixups reference the wrong commit.) Oh. I see. > >+ if (((percentage * PROGRESS_STEPS) - progress_next_step_i) > 0.0) > >+ progress_next_step_i++; /* ceil() */ > ... > >+ gint64 progress_step_duration = (timeout->end_timestamp_ms - timeout->start_timestamp_ms + PROGRESS_STEPS/2) / PROGRESS_STEPS; > > Two different rounding behaviors. It is (almost) correct as is. progress_next_step_i is the very next step (but not the current one). This was indeed slightly wrong, I fixed it to: /* calculate the next (not current) step: floor()+1 */ progress_next_step_i = ((int) (percentage * PROGRESS_STEPS)) + 1; progress_next_step_i = MIN (progress_next_step_i, PROGRESS_STEPS); progress_step_duration is just the duration of one step in milliseconds. The rounding is not really necessary (because PROGRESS_STEPS is 15 vs. thousands of milliseconds), but it doesn't hurt and is closer to the correct value. > >+ double percentage = ((double) (now - timeout->start_timestamp_ms)) / (timeout->end_timestamp_ms - timeout->start_timestamp_ms); > > We don't really need floating point in this code... Just use integer math on > the millisecond values; the user will not notice sub-millisecond rounding > errors. No. But it is easier to understand for me (and correct). > Also, if you computed progress_step_duration before the loop started, then > progress_next_step_i would be just > > (now - timeout->start_timestamp_ms) / timeout->progress_step_duration; Yes, but I cannot wrap my brain around this to see that progress_next_step_i is really correct. Using the @percentage double I can. Pushed fixup!
Created attachment 277468 [details] [review] intmath.patch Just as a strawman, here's the integer math patch. I think it's a bit simpler than the floating point code because it has one fewer calculation that you have to think about when looking at the code (eg, it removes 'percentage'). Either way we choose, I think it's worthwhile to: 1) precalculate progress_step_duration and store it in Timestamp 2) precalculate 'elapsed_ms' and store it in a const gint64 in handle_timeout()
(In reply to comment #7) > Created an attachment (id=277468) [details] [review] > intmath.patch > > Just as a strawman, here's the integer math patch. I think it's a bit simpler > than the floating point code because it has one fewer calculation that you have > to think about when looking at the code (eg, it removes 'percentage'). > > Either way we choose, I think it's worthwhile to: > > 1) precalculate progress_step_duration and store it in Timestamp > > 2) precalculate 'elapsed_ms' and store it in a const gint64 in handle_timeout() The patch is fine. Pushed fixup commit
danw, what do you think?
No danw response yet... Squashed branch looks fine to me.
Yes, looks good now. Except that the commit message still claims to allow sub-second precision, so that needs to be fixed. >- if (!timeout.value) { >+ if (remaining_ms <= 0) { nitpick: you know it can't be negative here (main), so use ==
(In reply to comment #11) > Yes, looks good now. Except that the commit message still claims to allow > sub-second precision, so that needs to be fixed. > > >- if (!timeout.value) { > >+ if (remaining_ms <= 0) { > > nitpick: you know it can't be negative here (main), so use == changed. Merged to master as: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=aaf0ef42dd26c0e67b27770c6c8e08c0e104998d