Bug 15853 - date-format-xparb spends 5% of TOTAL TIME in notify_check
Summary: date-format-xparb spends 5% of TOTAL TIME in notify_check
Status: RESOLVED DUPLICATE of bug 31197
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-06 01:33 PST by Eric Seidel (no email)
Modified: 2009-11-05 22:27 PST (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-11-06 01:33:59 PST
date-format-xparb spends 5% of TOTAL TIME in notify_check

We should move away from using notify_check and instead register for a UNIX signal for when the notify happens.  That would keep us from needing to poll.
Comment 1 Darin Adler 2007-11-06 08:47:39 PST
Some of this might be addressed by the patch I attached to bug 15852.
Comment 2 Geoffrey Garen 2007-11-07 12:02:54 PST
I'm not sure there is such a Unix signal. Unix signals are also a pretty bad way to do IPC. They can happen at any time, and very few operations are safe within a signal handler.

Maybe a better solution would be to check only every so often, instead of all the time. After all, we're checking for a rare event.
Comment 3 Eric Seidel (no email) 2007-11-07 23:08:30 PST
Well, regarding the unix signal: you get to pick your own.  Check out the API in /usr/lib/notify.h

As mjs and I discussed you would sign up for the signal and then have a global var cache of the timezone and two global var counters.  Assuming ++ is atomic on all platforms we care about, then you just have your sig handler increment one of the counters.  Every time we go to access the timezone info, we first check if the counters match.  If they don't then we assign the other counter to the signal counter's value, and then go and update the cached timezone info (using a normal localtime_r call, or whatever.

That should be thread safe.  All your doing is incrementing a global counter in a sig-handler, so it shouldn't matter what thread it's delivered on.  If another signal is delivered mid update, at any point during the update, the worst it should cause is that you end up updating with the same timezone twice.  Never should you miss a timezone update.

This method should be much faster than notify_check (which for whatever reason is ridiculously slow.  It uses a shared memory region and probably some locking of some form).  localtime_r also uses a mutex and notify_check, sadly.
Comment 4 Eric Seidel (no email) 2007-11-07 23:11:29 PST
The code looks something like this:

void timezoneChangedSigHandler()
{
     gTimeZoneNotifies++;
}

void doSomethingNeedingTheTimeZone()
{
    // current code would notify_check, instead we...
    if (gTimeZoneNotifies != gLastCachedNotify) {
        gLastCachedNotify = gTimeZoneNotifies;
        gTimeZone = // do the localtime_r dance to get the timezone
    }
    // use the timezone
}

I think that's safe... but it's also 11pm and I got a whole 2 hours of sleep last night or something... so it's possible I missed something obvious.
Comment 5 Alexey Proskuryakov 2009-03-24 10:58:07 PDT
Eric, is this bug still valid?
Comment 6 Eric Seidel (no email) 2009-03-24 11:28:52 PDT
I expect this bug does still have some validity.  We'd have to see how slow our date code is these days.  Someone would have to look at the output from:
run-sunspider --shark date-format-xparb
Comment 7 Geoffrey Garen 2009-11-05 22:27:50 PST
Reading this now, I like Eric's last idea, but this is probably obsoleted by bug 31197.

*** This bug has been marked as a duplicate of bug 31197 ***