Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to WebkitGTK 2.24.2 and workaround __clear_cache issue on ARM Cortex-A53 #114

Merged
merged 5 commits into from
Jun 25, 2019

Conversation

Kudo
Copy link
Member

@Kudo Kudo commented Jun 24, 2019

Summary

This PR includes three major changes:

  1. Enable JIT back
    Community reported sensible performance drop from the no-JIT version, so I'd like to enable JIT back.

  2. Upgrade to WebKitGTK 2.24.2.
    This seems to fix previous JSC crashes on Samsung S7 Edge.
    This version includes JIT new bytecode format as described from WebKit blog:
    https://webkit.org/blog/9329/a-new-bytecode-format-for-javascriptcore/
    After the major change, x86 JIT is not supported and arm32 support was contributed by WebKit community (Thanks to Igalia).
    From my understanding, original JSC crashes happen at operationLinkDirectCall(). After the new bytecode format, there is no direct link call from Baseline JIT. Since we've disabled DFG JIT and FTL JIT, there's no call flow that will hit to operationLinkDirectCall(). That is why no more similar crash happens.

  3. Workaround for ARM Cortex-A53 cache flush instruction issue:
    This is from V8's workaround and I believe it is worth to apply into JSC Android as well.
    https://codereview.chromium.org/1921173004
    ARM Cortex-A53 had some errata for original "cvau" instruction, and officially recommended to use
    "civac" instruction instead.
    LLVM compiler-rt's __clear_cache still uses "cvau" and my patch replaced to "civac".

Test Plan

  1. Run measure scripts on my Samsung Note 5.
  2. Provide an experimented version for community who previously reported JSC crash and seems no more crashes happened.

Measurement

Added "@kudo-ci/jsc-android@245459-no-dfg-jit" to previous measurement result.
The new result could compared to 241213-no-dfg-jit version.
There are some performance improvement from the comparison.
https://docs.google.com/spreadsheets/d/1hqX3ai-NCpN_J6YQDTKnKNBctWnMFA6EyOdVhPvwUas/edit#gid=193471288
Screen Shot 2019-06-24 at 11 46 00 PM
Screen Shot 2019-06-24 at 11 45 16 PM
Screen Shot 2019-06-24 at 11 45 08 PM
Screen Shot 2019-06-24 at 11 45 00 PM
Screen Shot 2019-06-24 at 11 44 56 PM

@Kudo Kudo requested a review from kmagiera June 24, 2019 15:57
@kmagiera
Copy link
Member

fantastic work @Kudo – will look into this one shortly

unsigned debugOffset() { return m_buffer.debugOffset(); }

-#if OS(LINUX) && COMPILER(GCC_COMPATIBLE)
+#if defined(CUSTOMIZE_REACT_NATIVE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this flag? It also does not seem to be set anywhere

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is my habit to enclose modifications by ifdef and it will be clear to see the scope we've patched.
It was first introduced from the getline patch for API 16.
Feel free to let me know if you prefer to remove this constant or apply this constant to all patches.
I will have another PR then.

if [[ "$ARCH_NAME" = "i686" ]]
then
JSC_FEATURE_FLAGS=" \
-DENABLE_JIT=OFF \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we still diable JIT in x86 builds?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After new bytecode format change from WebKit, 32bits platform JIT is not supported.
arm32 support is contributed by WebKit community (from Igalia)

There will be compile error if enable JIT for x86.

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, thanks for your replies, lets keep that constant, don't have strong preference. I think it is clear what the scope of changes is because all changes are listed in patches but I don't mind having it there either

@kmagiera kmagiera merged commit abdce96 into react-native-community:master Jun 25, 2019
@Kudo
Copy link
Member Author

Kudo commented Jun 25, 2019

I was thinking the constant being clear when editing a JSC C++ file.
It would be clear where we patched and don't need to refer back to patch file.
Anyway, as right now each patch file is relative small and I also found that the constant may break patch file during WebkitGTK upgrade.
I will try to remove them from next major changes.

Thanks for the feedback and help.

@rajivshah3
Copy link

Hi @Kudo ,

Thanks for the great work! Is this version compatible with RN 0.59.10? I noticed that the readme currently says to use 241213.x.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants