Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0007262opensim[MISC] DSGpublic2014-07-12 05:362014-07-13 11:57
Reportermikemig 
Assigned To 
PrioritynormalSeverityminorReproducibilitysometimes
Statuspatch includedResolutionopen 
PlatformLinuxOSUbuntuOS Version11.04
Product Versionmaster (dev code) 
Target VersionFixed in Version 
Summary0007262: tick counts need not be masked, either by 7FFFFFFF or 3FFFFFFF
DescriptionSome places in code mask Environment.TickCount by 7FFFFFFF and some places mask by 3FFFFFFF.

Also, some code tries to find an elapsed time by doing:
    int elapsed = currentTickCount & mask - previousTickCount;

...which is broken, as when:
     currentTickCount = 0x80000000
     previousTickCOunt = 0x7FFFFFFF,
   it yields 0x80000001

...it should be:
    int elapsed = (currentTickCount - previousTickCount) & mask;
   which yields the correct value of 1 for the above example

However, it shouldn't be necessary to mask the tick count at all. The previous statement can be simply:
    int elapsed = currentTickCount - previousTickCount;

It works even if none, either or both of current or previous tick count are negative. It breaks if the time difference is >= 2**31 ms, but so do any of the masked formulae. And masking by 3FFFFFFF breaks if the difference is >= 2**30 ms.
TagsNo tags attached.
Git Revision or version numbereea3be9b17bcece898a6e5a599366177ab2564f1
Run Mode Grid (1 Region per Sim)
Physics EngineBulletSim
Script Engine
EnvironmentMono / Linux64
Mono Version2.10
Viewer
Attached Filespatch file icon tickcountmask.patch [^] (12,510 bytes) 2014-07-12 05:38 [Show Content]

- Relationships

-  Notes
(0026469)
Robert Adams (administrator)
2014-07-12 07:53

This problem has been noticed before and some routines were added to Util to centralize the computation (Util.EnvironmentTickCount(), Util.EnvironmentTickCountSubtract() and Util.EnvironmentTickCountSubtract()). It would be better if the patches used those central routines.
(0026470)
mikemig (reporter)
2014-07-12 11:21

I noticed that some attempt was made to centralize a computation. But I was trying to point out that the masking is unnecessary and is actually detrimental. The implemented Util routines can only handle intervals of 2**30 ms. And although this interval is approx 12.5 days, I've seen failures. Removing the masking gives a range of 25 days and is simpler.

There is even some really broken code that does:
    int elapsed = currentTickCount & 0x7FFFFFFF - previousTickCount;
which breaks even for small intervals every 25 days, ie, whenever previoustickCount was positive and currentTickCount is negative.

So why not simply get rid of all the masking? Then the above becomes:
    int elapsed = currentTickCount - previousTickCount;
which works for intervals up to 25 days regardless of what time of the month they span.

Yes we could convert it all to use the central routines after removing all the masking code or get rid of the central routines altogether as they would simpy return the Environment.TickCount or simply subtract.
(0026471)
Robert Adams (administrator)
2014-07-13 06:42

The change to use centralized routines was to fix some bugs: there were places where difference arithmetic was failing when the positive to negative rollover happened. The environment time has too many gotchas when used for time intervals. Please centralized any use of it.

Also, any code measuring time that might be longer than a day SHOULD NOT be using environment tick count. Just making the bug show up in 25 days instead of 12 days only hides the problem of code making the wrong design decision.
(0026472)
mikemig (reporter)
2014-07-13 11:57

My point was in essence, that the correct fix back when would have been to remove all masking of tickcounts and simply subtract instead of adding in all the masking stuff. And IMO (can be taken either way), other people won't be aware of the Util tick count stuff (there are cases in OpenSim where direct tick counts are used properly), as the proper way to measure an interval is to simply subtract and don't use any masking. And of course if the Util stuff is eliminated, then all uses of tick count can be found by searching for Environment.TickCount and not have to search for both Environment.TickCount and EnvironmentTickCount. But I'm not opposed to leaving the Util stuff in, to me it just seems pointless.

My point about 12days vs 25days was not attempting to sweep anything under the rug, I was merely pointing out that by not using masks, the tick count is more robust than when using masks. I was not going to investigate every use of tick count for this change and determine if tick count was appropriate for a given use case or not, as it shouldn't be used for any interval that can come close to overflowing in the worst case. Those could be separate changes.

- Issue History
Date Modified Username Field Change
2014-07-12 05:36 mikemig New Issue
2014-07-12 05:38 mikemig File Added: tickcountmask.patch
2014-07-12 05:39 mikemig Status new => patch included
2014-07-12 07:53 Robert Adams Note Added: 0026469
2014-07-12 11:21 mikemig Note Added: 0026470
2014-07-13 06:42 Robert Adams Note Added: 0026471
2014-07-13 11:57 mikemig Note Added: 0026472


Copyright © 2000 - 2012 MantisBT Group
Powered by Mantis Bugtracker