Bug 30681

Hold this a bit. I'll check tomorrow, but I have a gut feeling that we changed the time functions In Qt not to use Open C but use Symbian APIs instead. We have the ECMA/Date tests disabled for Embedded Linux and Symbian as HW targets will fail due to slow execution time.

(In reply to comment #1) > Hold this a bit. I'll check tomorrow, but I have a gut feeling that we changed > the time functions In Qt not to use Open C but use Symbian APIs instead. We > have the ECMA/Date tests disabled for Embedded Linux and Symbian as HW targets > will fail due to slow execution time. Yes, I was also told that QDateTime and friends are being fixed up. :) But Javascript/WTF/DateMath.cpp is not using Qt at all for its Date/Time manipulation and goes straight to the POSIX APIs. You can check this yourself if you see who calls gregorianDateTimeToMS() and msToGregorianDateTime() in DateMath.cpp.

Created attachment 41679 [details] proposed patch Attaching patch. Two functions in DateMap.cpp use Symbian OS APIs directly for estimating Daylight savings correction. - Verified that ECMAscript Date tests all pass in Symbian emulator.

Looks good for me. I checked and there is no overlap, infact we will change some of the functions to be more in line with this patch :)

Performance improvement: Sunspider date-totfe completes in 642ms with the above patch on an N97 vs. 12834ms previously using openC/PIPS implementation (the latter time is particularly bad because looking up timezone databases leads to lots of file I/O and client-server IPC on Symbian OS)

Corrected numbers: 5111ms for both date tests in Sunspider after the patch, 26682ms before. So about 5x faster.

Comment on attachment 41679 [details] proposed patch I don't see how this could be right. I'll try to review later when I have more time, but this seems obviously wrong on my first reading.

Comment on attachment 41679 [details] proposed patch Right, seems that this would trivially break getUTCOffset() by making it include DST correction. Not all callpaths automatically add the DST offset later. If Qt has better implementation, you could just switch to those (from the posix ones, with appropriate ifdefs).

Antti, thanks for your time and review comments. My clarifications are below: (In reply to comment #8) > (From update of attachment 41679 [details]) > Right, seems that this would trivially break getUTCOffset() by making it > include DST correction. Not all callpaths automatically add the DST offset > later. a) getUTCOffset() is only called from two functions in the same file: gregorianDateTimeToMS() and its inverse gregorianDateTimeToMS(). It is called from nowhere else for any port. In both places it is added to getDSTOffset(). I have changed both, so that their sum is consistent. b) The reason getUTCOffset() and getDSTOffset() always go together is because of the ECMAscript spec being written that way. The code mirrors the spec very nicely. http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-262.pdf Section 15.9.1.9 describes the only place in the EMCA spec where LocalTZA and DaylightSavingsTA are used. They are added to each other. DaylightSavingsTA == getDSTOffset() , and LocalTZA == getDSTOffset(). > If Qt has better implementation, you could just switch to those (from the posix > ones, with appropriate ifdefs). c) I am hesitent to add Qt dependecy to Javascript/WTF/* because this code is entirely Qt-unware. It also use C/C++ primitive types like double all over the place, so any work to introduce something like qreal and other Qt baggage would be quite intrusive and might make the code less readable for other ports. Thanks again for your time and help!

(In reply to comment #9) > a) getUTCOffset() is only called from two functions in the same file: > gregorianDateTimeToMS() and its inverse gregorianDateTimeToMS(). It is called > from nowhere else for any port. In both places it is added to getDSTOffset(). I > have changed both, so that their sum is consistent. At least a GregorianDateTime constructor in DateMath.h calls getUTCOffset() and expects it not have DST included. Anyone could add mode code that relies on getting the correct values from these functions. If there truly is no valid reason to get UTC and DST offsets separately then there probably should be only one function for returning the combined value. > c) I am hesitent to add Qt dependecy to Javascript/WTF/* because this code is > entirely Qt-unware. It also use C/C++ primitive types like double all over the > place, so any work to introduce something like qreal and other Qt baggage would > be quite intrusive and might make the code less readable for other ports. > > Thanks again for your time and help! You could probably put all Qt calls to a single file that does not expose Qt to the rest of the JSC. But you are right that avoiding the dependency is probably good idea if it does not exist already.

(In reply to comment #10) > At least a GregorianDateTime constructor in DateMath.h calls getUTCOffset() and > expects it not have DST included. Anyone could add mode code that relies on > getting the correct values from these functions. > If there truly is no valid reason to get UTC and DST offsets separately then > there probably should be only one function for returning the combined value. My bad, I missed DateMath.h. That GregorianDateTime c'tor is also adding a presumed DST offset of 1 hour there too, so I think I will make a patch that will prevent getUTCOffset() from being called by itself and wrap it all cleanly. Thanks Antti!

(In reply to comment #9) > > If Qt has better implementation, you could just switch to those (from the posix > > ones, with appropriate ifdefs). > > c) I am hesitent to add Qt dependecy to Javascript/WTF/* because this code is > entirely Qt-unware. It also use C/C++ primitive types like double all over the > place, so any work to introduce something like qreal and other Qt baggage would > be quite intrusive and might make the code less readable for other ports. I don't think there's a problem using Qt functions in JavaScriptCore/WTF, we're doing that all over the place already. On the other hand your patch is realtively simple, too :) If it makes the patch easier and of course the code faster I'd say don't hesitate with using Qt functions there. It might also help to get more consistent behaviour across all Qt ports.

Created attachment 44235 [details] another fix Added version that accounts for DST changes. DST is checked from timezone server on Symbian.

(In reply to comment #13) > Created an attachment (id=44235) [details] > another fix > > Added version that accounts for DST changes. DST is checked from timezone > server on Symbian. Janne, I am sorry but this is not good performance-wise. As you probably know client<->server communication in Symbian is a very slow process that requires a context-switch for each API call. And that's just part of the problem, since the concerete implementations of Connect(), DoWork(), Close() are time consuming by themselves [1]. So use of 2 Connect(), 2 Close() , and 2 TZ server API calls is not acceptable in speed critical code like JSC's Date/time implementation. Did you get the chance to time this on an N97 using Sunpsider date tests? On a related front, I previously noticed in reading the ECMAscript spec [Comment #9] that it's flexible about how an implementation does DST isolation. My understanding is that as long as the (functional) compliance tests pass, some liberties can be taken. If anyone disagrees, please comment. Therefore, another plan of attack _might_ be .. Patch 1: Remove the use of getUTCOffset() in the c'tor in DateMath.h and also makes sure that the affected member variable isn't incorrectly used later on/has invalid data. IIRC, only some ports use that member variable. Patch 2: Follow the suggestion in Comment #10 about "one function that returns a combined (sum) value" of getUTCOffset() + getDSTOffset(). Then it would be easy to make that wrapper function use either a new flag HAVE_WEAKDSTISOLATION, or PLATFORM(SYMBIAN), or both to return the value returned by User::UTCoffset(). The advantage of the above approach is that it requires zero Symbian server API usage and hence will be as efficient as a port's Date code ought to be. The disadvantage is that the UTC offset is for the current time, not an arbitrary time in the past or future. But if the ECMAscript functional compliance tests pass, the latter shouldn't be a problem, IMHO. [1] Page 14-19 : http://www.symbianresources.com/tutorials/advanced/clientserver/Client-ServerFramework.pdf

> context-switch for each API call. And that's just part of the problem, since > the concerete implementations of Connect(), DoWork(), Close() are time > consuming by themselves [1]. So use of 2 Connect(), 2 Close() , and 2 TZ > server API calls is not acceptable in speed critical code like JSC's Date/time > implementation. Did you get the chance to time this on an N97 using Sunpsider > date tests? Results from 5800 express music: date-format-tofte + format-xparb = date total Before 5543ms + 24228ms = 29772ms After 2386ms + 2534ms = 4921ms Have you actually checked Context Switch times on newer devices ? it is minimal. Here is context switch time from my E71: Inter-thread context switch: 6.270000 us / round-trip (100000 round trips in total) Inter-process context switch: 6.860000 us / round-trip (100000 round trips in total)

(In reply to comment #15) > Results from 5800 express music: > date-format-tofte + format-xparb = date total > > Before 5543ms + 24228ms = 29772ms > After 2386ms + 2534ms = 4921ms > Wow, in that case I eat my words. (Yes, I see FTC trace of .Connect(), Close() and server API calls almost every day and they are really slow). I will make a phone build using the patch, try it out and report back. Thanks!

> Wow, in that case I eat my words. (Yes, I see FTC trace of .Connect(), Close() > and server API calls almost every day and they are really slow). > > I will make a phone build using the patch, try it out and report back. > Slow is relative, we are talking about seconds here :) FTC resolution is insane, I'm amazed by the tool. .Connect() would be really slow in case where server is not running and having to start the server process. I would assume TZ to be always running, could be wrong. Problem is that there really aren't other alternatives when using pure Symbian APIs. There is one call that could be made in TLocale but that is said to be deprecated in the header. And for Open C , I don't have their source code so I can't say what they are doing to spend so much time on those functions. Best alternative after all would be to fix Open C and leave datemath.cpp as is. Try it out and see what numbers you get. --- Measuring just the context switch doesn't really bare any meaning.. here is some more interesting numbers for this case. Notice the huge difference, this is what I was referring to. These numbers haven't been measured with FTC so they have some extra time from instrumentation. Nokia E61: Synchronous IPC function with one argument and result: 143,550000 us / call (20000 calls in total) Synchronous IPC function with buffer[32] argument read: 161,900000 us / call (20000 calls in total) Nokia E71: Synchronous IPC function with one argument and result: 7,550000 us / call (20000 calls in total) Synchronous IPC function with buffer[32] argument read: 13,250000 us / call (20000 calls in total)

(In reply to comment #17) > Slow is relative, we are talking about seconds here :) Indeed , _seconds_ . On a 434MhZ ARM11. ;) Anyways, I was able to verify that with your patch, the number on N97 was dramatically better than with the existing openC/PIPS release. Please feel free to open the patch for review when you are ready.

(In reply to comment #13) > Created an attachment (id=44235) [details] > another fix > > Added version that accounts for DST changes. DST is checked from timezone > server on Symbian. Janne, the patch looks like it should be marked for review :)

style-queue ran check-webkit-style on attachment 44235 [details] without any errors.

Comment on attachment 44235 [details] another fix It's unfortunate that this is abstracted via #ifdefs instead of functions. If we used functions then we could at some later date move those functions to another file.

Comment on attachment 44235 [details] another fix Killed the patch. I've found out when doing making the same patch for Qt that 3.1/3.2/5.0 all have major differences in API for timezone server and would cause this patch to fail to compile on 3.1. Problem is that 3.1 version of the API cannot be used like this to get DST at all. Build variant for 3.1/3.2/5.0/future needs to be added if the code will stay on webkit side. I think datemath needs to call datetime as already suggested in the comments.

I've also contacted Open C guys who promised to take a look at their performance of their posix time functions.

(In reply to comment #23) > I've also contacted Open C guys who promised to take a look at their > performance of their posix time functions. Got a patch from Open C guys and testing it out. If it works out fine I guess we could close this issue. Problem is that the fix won't make it into their next release and I have no idea when newer release will be coming out.

I finally managed to run the tests on patched Open C (got several non-working version of it). My testsuite that I built explicitly to be the reference for this patch didn't start after the patched Open C which isn't such nice thing as now I don't have real baseline to go with. tofte + xbarb 2933ms + 10567ms = 13500ms which is half the original time, but still lacks from my patch. To fix the patch will make the time taken to skyrocket in that or something more drastic changes is required for Symbian. Using Qt is no option here as QDateTime doesn't offer API to get DST. Maybe if QDateTime API would be changed. And the same bug is there too..

Open C fixes have been tested and speed is at acceptable level. I'll close the issue for now and check back on once we have new Open C version released.

(In reply to comment #26) > Open C fixes have been tested and speed is at acceptable level. I'll close the > issue for now and check back on once we have new Open C version released. For record keeping only: Internal OpenC bug for this fix is TSW VPOL-7WS7P2. (Janne, please correct if necessary)