58310 - Create PRTimeInSeconds() to consolidate all duplicate and costly implementations PRTimeToSeconds() and SecondsFromPR

Status: RESOLVED WONTFIX

(NSPR :: NSPR, defect, P3)

Description

[(not reading, please use [e-mail] instead)]

see http://lxr.mozilla.org/seamonkey/search?string=convertPRTimeToSeconds

there's also a variation of it in nsProfile.cpp

wtc, would this be something to add to NSPR?

Comment 1

[Wan-Teh Chang]

No.  PRTime is 64-bit for a reason.  A 32-bit
time_t (number of seconds since 1970) will overflow
in 2036 or 2038.

Comment 2

[(not reading, please use [e-mail] instead)]

the variant of the code (in nsProfile.cpp) uses the number of seconds as an
argument to srand(), which takes a uint.  I'm not concerned about overflow.

the other callers of convertPRTimeToSeconds() may be concerned.  I'll investigate. 

Comment 3

[gordon]

Changing component from Network:Cache, because it seems unrelated.

Comment 4

[neeti]

-->Networking:HTTP

Comment 5

[Andrew Taylor]

I can't find any references to convertPRTimeToSeconds.  Convertions to seconds
seems to be done with the LL_* macros in nsProfile.cpp, which is pretty straight
forward.  I don't think there is any need to consolidate with other conversions.

Marking INVALID.

Comment 6

[benc]

darin: should this be in HTTP?

Comment 7

[Alfred Kayser]

*** Bug 223804 has been marked as a duplicate of this bug. ***

Comment 8

[Alfred Kayser]

Actually they are still there:
  http://lxr.mozilla.org/seamonkey/search?string=PRTimeToSeconds
and 
  http://lxr.mozilla.org/seamonkey/search?string=SecondsFromPR

But, the NSPR owners (still) refuse to have a PRUint32 PRTimeToSeconds (because
of the overflow).

So, currently the local implementors (about 20!) are now taking the risk of
overflow (in 2037 or 2100 depending on the signness of the 32bit integer).

Also, as measured in the caching code, the combination of
SecondsFromPRTime(PR_Now()) is costly (taking a fair chunk of the time spent in
caching code). 

Note, Bug 75001 (Too many calls to PR_Now in HTMLContentSink::IsTimeToNotify) is
another example of the impact of 'SecondsFromPRTime(PR_Now())'

So, a central implemented PRUint64 PR_NowSeconds(), optimized for this specific
function would reduce code as well as improve performance.  
The callers can then still convert to PRUint32 if they really want to...

May be, because it is used so often a central timer maintaining the current
'now' in seconds value? So that all callers can just retrieve this value would
be a trick to reduce the number of 'gettimeofday' calls (which are, especially
in Unix world, are costly)?

Comment 9

[Alfred Kayser]

Reopening: Consolidation still has to take place ! Even if not in NSPR!

Comment 10

[Alfred Kayser]

Other aliases of uses of PR_Now with conversion to seconds are at:
http://lxr.mozilla.org/seamonkey/search?string=NOW_IN_SECONDS
http://lxr.mozilla.org/seamonkey/search?string=NowInSeconds
http://lxr.mozilla.org/seamonkey/search?string=PRTime2Seconds
http://lxr.mozilla.org/seamonkey/search?string=TimeInSecondsFromPRTime

http://lxr.mozilla.org/seamonkey/search?string=ssl_Time() 
/* returns an unsigned int containing the number of seconds in PR_Now() */

Or just as: PRInt32 elapsedSecs = (PR_Now() - startTime) / PR_USEC_PER_SEC;
Or even: nsInt64 currentTime = nsInt64(PR_Now()) / nsInt64(1000000);

So, a great number of locations where a PR_NowSeconds() is needed, but all
implement there own version.

Note, furthermore that PR_Now is generally implemented as follows:
gettimeusingOSfunction(&struct)
value = struct.seconds*1000000 + struct.milliseconds*1000; 
or variations thereof.

Even worse, for Mac, PR_Now is only accurate at seconds.



So, proposal is to extend NSPR with PR_NowSeconds, so that calculation and
conversion of microseconds is not needed, and the caller doesn't have to convert
is back to seconds again.

PRInt64 PR_NowSeconds(void)
{
    struct timeval tv;
    PRInt64 s;

    GETTIMEOFDAY(&tv);
    LL_I2L(s, tv.tv_sec);
    return s;
}

Comment 11

[Alfred Kayser]

From bug 243486: 
* The SecondsFromPRTime(PR_Now()) combination is costly: The extra conversions
on top of gettimeofday are redundant if we just want to have the current time in
seconds...

Comment 12

[Alfred Kayser]

Attachment: List of all occurances of PR_Now conversions into seconds...

Comment 13

[Alfred Kayser]

Some timing results:
      PR_Now()/PR_USEC_PER_SEC  1000000 times:      110ms, 0.110000 us/call
                 PR_NowSeconds  1000000 times:       65ms, 0.065000 us/call

So, for windows PR_NowSeconds is twice as fast as PR_Now()/PR_USEC_PER_SEC.
For Unix case (based on ftime), the times are resp. 0.343us and 0.240us, so
there is saves even more, because all the LL calculations are skipped.

Comment 14

[benc]

Can we change the component? This hardly seems http-specific.

Comment 15

[benc]

-> defaults.

Thanks.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:KaiE, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 17

[Alfred Kayser]

In current source code baseline, PRTimeInSeconds no longer exists, so this bug is no longer relevant.