hermes icon indicating copy to clipboard operation
hermes copied to clipboard

Date 800x slower than JSC

Open burakgormek opened this issue 2 years ago • 67 comments

Bug Description

Date so slow that it can be freeze app. Impossible to use third party date library(much worse perf, 1800x). I found that local date is a problem. UTC date still slow but not 800x, just 8x slower than JSC which can be ignored.

The code outputs: JSC: 0.2605330003425479 Hermes: 166.44708999991417 Hermes UTC: 1.6460870001465082

  • [x] I have run gradle clean and confirmed this bug does not occur with JSC

Hermes version: Bundled version with RN 0.71.3 React Native version (if any): 0.71.3 OS version (if any): iPhone 14 (simulator) Platform (most likely one of arm64-v8a, armeabi-v7a, x86, x86_64):

Steps To Reproduce

  1. Create initial app with latest RN version.
  2. Add somewhere the code that down below. Look console to see the numbers.

code example:

var a = performance.now();
Array(1000)
  .fill(0)
  .map(() => {
    new Date(new Date().setMonth(1));
    // fast utc version 
    // new Date(new Date().setUTCMonth(1));
  });
var b = performance.now();

console.log(b - a);

The Expected Behavior

Fast Date operations.

burakgormek avatar Mar 03 '23 11:03 burakgormek

Thank you for reporting this. It looks like an actual problem and is likely caused by us not caching the timezone information.

As an experiment, we changed the system timezone while the JS process was running to see whether it would invalidate the timezone. Hermes displayed the correct new timezone, while JSC and v8 displayed the original one. It is not entirely clear which behavior is preferable.

Could you try a similar experiment in your mobile app?

tmikov avatar Mar 04 '23 02:03 tmikov

I can confirm this, I previously used spacetime js to simplify date comparisons in my expo managed app. With the newest release of expo 48 we transitioned to hermes engine and had to remove all spactime js uses because just 150 items slowed down the app to a standstill for several seconds whilst running date comparisons.

This is now our code:

image

And this is also slower than it was using jsc, by a large margin.

ljukas avatar Mar 06 '23 06:03 ljukas

@tmikov this is a major problem for anything which relies on dates to generate unique identifiers or localized translations..

Both iOS and android have mechanisms to notify the app when there is a timezone change. https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1622992-applicationsignificanttimechange https://developer.android.com/reference/android/content/Intent#ACTION_TIMEZONE_CHANGED

IMO, caching the timeszone info and then updating that info only when there is an actual timezone change could be a potential solution here.

rpopovici avatar Mar 06 '23 13:03 rpopovici

We have exactly the same issue. Because of it we cannot switch to Hermes engine unfortunately.

I run this function in a sorting loop (~500 items in a list)

const isSameDay = (date1: Date, date2: Date) =>
  date1.getFullYear() === date2.getFullYear() &&
  date1.getMonth() === date2.getMonth() &&
  date1.getDate() === date2.getDate();

Here is a preview:

And then I tried to use isSame function form dayjs and it took ~15500ms!!! to complete sorting

DePavlenko avatar Apr 27 '23 14:04 DePavlenko

We agree that this needs to be fixed to match the behavior of the other engines. Unfortunately we don't have concrete plans on when exactly we can work on the fix yet. Meanwhile a community contribution would be welcome.

tmikov avatar Apr 27 '23 19:04 tmikov

Thank you for the quick response @tmikov! I just wonder how people handle it in their projects at the moment (don't use Hermes?). It's quite hard to find any opened issues or articles about the problem but I think almost every project has some interactions with dates

DePavlenko avatar Apr 28 '23 08:04 DePavlenko

Thank you for the quick response @tmikov! I just wonder how people handle it in their projects at the moment (don't use Hermes?). It's quite hard to find any opened issues or articles about the problem but I think almost every project has some interactions with dates

The "solution" in our case is indeed not using Hermes atm.

harrigee avatar Apr 28 '23 08:04 harrigee

@tmikov The simplest most straight forward solution here would be to expose an API from hermes which will cache the timezone info on demand instead of doing it automatically on every call.

rpopovici avatar Apr 28 '23 08:04 rpopovici

@rpopovici are there APIs for updating the cached timezone information in JSC? In other words, if a RN app is running in JSC and a system timezone change occurs, how can you force it to refresh its cache? If there is no such API, how can the app handle timezone changes at all?

tmikov avatar May 18 '23 02:05 tmikov

@tmikov if relying on timezone change events is a deviation from the standard behaviour, then trying to debounce the timezone info every second could be an acceptable compromise solution which could alleviate the perf bottleneck and is better than JSC or v8 since these two won't update the timezone data at all.

Also the timezone debouncing solution can be easily exposed through a hermes runtime config flag and then you can keep both behaviours and let the user decide if they need super accurate timezone info or they can settle for something less accurate but more performant

rpopovici avatar May 18 '23 06:05 rpopovici

Listening for NSSystemClockDidChangeNotification or NSSystemTimeZoneDidChange can be done directly from PlatformIntlApple.mm using objective-c blocks. That should work without any side effects from C++

id __block token = [
    [NSNotificationCenter defaultCenter] addObserverForName: NSSystemTimeZoneDidChangeNotification
    object: nil
    queue: nil
    usingBlock: ^ (NSNotification * note) {
        // do stuff here, like calling a C++ method
    }
];

Don't forget to remove the subscription for NSSystemTimeZoneDidChange event on destructor. https://developer.apple.com/documentation/foundation/nsnotificationcenter/1411723-addobserverforname?language=objc

[[NSNotificationCenter defaultCenter] removeObserver:token];

This should allow you to update the cached timezone data only when NSSystemTimeZoneDidChange event fires.

rpopovici avatar May 20 '23 06:05 rpopovici

@rpopovici this indeed looks like an attractive solution. What is the thread safety of the callback, i.e. in what thread will it be invoked?

I suspect the safest thing to do here is to set a per-runtime atomic flag and check it later.

tmikov avatar May 22 '23 16:05 tmikov

@tmikov When queue: nil, the block runs synchronously on the posting thread, but than can be accommodated to run on different NSOperationQueue if that's necessary.

I suspect the safest thing to do here is to set a per-runtime atomic flag and check it later. Yes

rpopovici avatar May 22 '23 16:05 rpopovici

Hmm, unfortunately Hermes doesn't own a thread or an NSOperationQueue. It executes in the context of an arbitrary thread provided by an integrator. Adding functionality like this is never as simple as it appears.

So, let's clarify the scope of the problem. Other engines like v8 and JSC cache the timezone permanently with no ability to update it, even if the system timezone changes. Apparently RN developers find that behavior acceptable. It seems like we can duplicate that behavior in Hermes. Is that a reasonable conclusion?

tmikov avatar May 22 '23 17:05 tmikov

@tmikov can't speak for others but it seems ok for our case.

rpopovici avatar May 22 '23 18:05 rpopovici

Any updates on this? Still blocking us from switching to hermes

morgan-cromell avatar Jun 15 '23 16:06 morgan-cromell

I think this seems like the most likely culprit destroying my app performance with hermes. We have a fair amount of date logic since part of the app is a todo list. The app runs 4-6x slower on hermes than on JSC or v8.

Hopefully this can get bumped up in priority. It seems like it's impacting a fair number of projects. There are probably many projects using hermes by default now that are suffering poor performance because of this.

Perhaps some basic performance tests should be added to the hermes test suite to hopefully catch app breaking performance regressions like this before they get out to production apps.

evelant avatar Jul 09 '23 20:07 evelant

@evelant do you have a repro of your problem or is this just a guess?

How many date calculation per second is your app performing and what kind of calculation? What does "runs 4-6x slower" mean? Is the update rate 6 times slower? Is your app performing non-stop date calculations in a loop?

tmikov avatar Jul 10 '23 10:07 tmikov

Unfortunately I don't have a repro, it's just a guess.

By 4-6x slower I mean that user-perceived responsiveness is 4-6x slower. Everything in the app takes 4-6x longer when running on hermes.

My best guesses so far at likely culprits are:

  • Date logic performance. Depending on the user the app could be performing hundreds or more date calculations on loading and some interactions.
  • Proxy performance. The app relies heavily on mobx which uses proxies to make objects observable for efficient react renders and derived computations.
  • JSI performance. The app uses react-native-skia and react-native-mmkv heavily. Those are both JSI libraries.
  • JSON parse/stringify performance. Some users have a lot of data, if serializing/deserializing that is slower with hermes that would add to the issue.

Sorry I don't have more than a guess at the moment. I don't currently have the time to really pick apart the app to narrow it down. We're just going to run on JSC or v8 until there's more room to investigate.

evelant avatar Jul 10 '23 12:07 evelant

@tmikov you can't control how many calls / sec are out there. Most modern apps use translation and text formatting libraries which rely heavily on Date APIs. UUID, crypto key generation or time stamping rely on Date APIs. Just a simple sorting algorithm for a small item list can freeze your app with hermes.js when date comparison is involved.

This is why Date needs to be blazing fast. It is used everywhere.

rpopovici avatar Jul 10 '23 13:07 rpopovici

@rpopovici Agreed. For example our app by default groups tasks by day for a calendar view. If someone has a couple hundred tasks we're tripping over exactly the issue @DePavlenko described above where a simpleisSameDay calculation can be 2600x slower on hermes.

evelant avatar Jul 10 '23 13:07 evelant

@evelant help me understand, why do you need to perform local time calculations for hundreds of tasks? Ordinarily, local time would only be used for display, and all logic would use UTC?

tmikov avatar Jul 12 '23 20:07 tmikov

@tmikov Most of the logic uses UTC, but we also use libraries like date-fns that might or might not use Date instances under the hood. As others have said however, it doesn't really matter -- it's not an app problem since the app is fine on JSC and v8, it's a hermes problem. If using particular date libraries or accidentally doing some date calculations using Date objects instead of UTC timestamps can cause such a massive slowdown that's a serious bug in hermes, you can't expect app/library developers to work around that just for hermes.

evelant avatar Jul 12 '23 20:07 evelant

Can someone please provide a real example (not a synthetic one) demonstrating the problem? The already synthetic examples are illustrative, but for real optimization work we need examples of actual useful code that needs its performance improved.

tmikov avatar Jul 19 '23 18:07 tmikov

Here you go:

image

image

Doing date operations on about 600 objects in an array takes 4.2s, which is extremely slow

Spacetime is this lib: https://github.com/spencermountain/spacetime

ljukas avatar Jul 20 '23 09:07 ljukas

Here is the same function, and results using just Date objects.

image

image

Which means this takes 4-5 frames to calculate, which is incredibly slow even that

ljukas avatar Jul 20 '23 09:07 ljukas

Testing shows that creating spacetime objects is whats taking 95% of the time in hermes, so I tested it using node locally and here is the same test pretty much.

image

image

Just to show you the difference.

Ill accept that the 85ms is in big reason because we are running in development mode.

But here we can compare creation of 600 spacetime objects in hermes takes 4 seconds. Creating 600 spacetime objects in node locally takes 1.5 ms

Thats 2600x slower.

I hope this is enough to show how extremely slow hermes is with date calculations

ljukas avatar Jul 20 '23 09:07 ljukas

@ljukas help me out here. I attempted to reproduce your latest example, but I got very different results. I started by installing Spacetime in an empty directory:

mkdir test && cd test
npm install spacetime

Then created a file called node-test.js with the following contents:

const spacetime = require("spacetime");

const now = Date.now();
for(let i = 0; i < 600; ++i)
    spacetime('2012-05-24');
const end = Date.now();

(typeof print != "undefined" ? print : console.log)('ms', end - now);

and executed it in the following way:

$ node node-test.js
ms 37
$ node --jitless node-test.js
Warning: disabling flag --expose_wasm due to conflicting flags
ms 66

Please observe that the execution time was not 1.5ms but instead 37 ms and 66 ms without JIT for a fair comparison with Hermes.

Then I packaged Spacetime and the test file into a bundle to run in Hermes in the following way:

cat node_modules/spacetime/builds/spacetime.cjs <(tail -n +3 node-test.js) > bundle.js

This packages together the library itself and test file. The tail call removes the unnecessary require().

Then I ran the packaged file with hermes, v8 and jsc, with and without JIT:

$ hermes bundle.js
bundle.js:34:7: warning: the variable "console" was not declared in arrow function "quickOffset"
      console.warn("Warning: couldn't find timezone " + s.tz);
      ^~~~~~~
bundle.js:211:16: warning: the variable "Intl" was not declared in arrow function "safeIntl"
    if (typeof Intl === 'undefined' || typeof Intl.DateTimeFormat === 'undefined') {
               ^~~~
ms 57

$ v8 bundle.js
ms 37

$ v8 --jitless bundle.js
Warning: disabling flag --expose_wasm due to conflicting flags
ms 64

$ jsc bundle.js
ms 30

$ jsc --useJIT=0 bundle.js
ms 71

Please note that Hermes is actually slightly faster in this case than jitless v8 and jitless JSC and is less than 2x times slower than the jit.

This is not even remotely close to the 2600x number you reported. Can you please explain what I did wrong, so I can reproduce your results?

tmikov avatar Jul 22 '23 03:07 tmikov

Additionally I compiled Hermes with Intl enabled, which made it 2x slower, but still a far cry from 2600x and certainly in the same ballpark as jitless v8 and JSC:

hermes bundle.js
bundle.js:34:7: warning: the variable "console" was not declared in arrow function "quickOffset"
      console.warn("Warning: couldn't find timezone " + s.tz);
      ^~~~~~~
bundle.js:211:16: warning: the variable "Intl" was not declared in arrow function "safeIntl"
    if (typeof Intl === 'undefined' || typeof Intl.DateTimeFormat === 'undefined') {
               ^~~~
ms 114

So, to reiterate, at this point we don't have a non-synthetic example demonstrating the problem. Invoking the Spacetime constructor in a loop with a string parameter actually demonstrates that Hermes is competitive, so we need something else.

tmikov avatar Jul 22 '23 03:07 tmikov

As a non-synthetic example, displaying hundreds of data points on a graph. The Intl options passed to toLocaleTimeString might are also different in case of a more complex batched group of data points. So having to workaround all the logic in UTC only due to Hermes is not ideal.

On my machine the rough numbers I'm getting with code below, show almost a 10x difference:

  • V8 (M1, nodejs cli) - 30ms
  • Hermes (M1, react-native iOS simulator) - 235ms

This is only amplified with additional Intl API calls for every data point so these 1000x numbers can be realistic.

const t0 = performance.now();

function convertIsoToHour(isoTimestamp) {
  return new Date(isoTimestamp).toLocaleTimeString([], { hour: '2-digit', minute: '2-digit' });
}

const arrayOfObjects = [
  ...Array(500)
    .fill(0)
    .map((_, i) => ({
      id: `${i}`,
      name: `name${i}`,
      timestamp: '2021-09-15T18:30:00.000Z',
    })),
];

const plotData = [];

for (let i = 0; i < arrayOfObjects.length; i++) {
  const obj = arrayOfObjects[i];
  const hour = convertIsoToHour(obj.timestamp);

  plotData.push({
    id: obj.id,
    name: obj.name,
    hour,
  });
}

const t1 = performance.now();
console.log(`Call took ${t1 - t0} milliseconds.`);

rrebase avatar Jul 22 '23 13:07 rrebase