Skip to content
New issue

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

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

Already on GitHub? # to your account

GC concurrent issue potential fix #1

Merged
merged 2 commits into from
Dec 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 125 additions & 0 deletions patches/jsc_fix_concurrent_gc_issue.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
--- target-org/webkit/Source/JavaScriptCore/ChangeLog 2019-09-23 18:14:45.000000000 +0800
+++ target/webkit/Source/JavaScriptCore/ChangeLog 2019-12-18 08:36:29.000000000 +0800
@@ -1,3 +1,40 @@
+2019-10-18 Yusuke Suzuki <ysuzuki@apple.com>
+
+ [JSC] Make ConcurrentJSLock Lock even if ENABLE_CONCURRENT_JS=OFF
+ https://bugs.webkit.org/show_bug.cgi?id=202892
+
+ Reviewed by Mark Lam.
+
+ We are using ConcurrentJSLock to guard data structure against concurrent compilers.
+ But these data structures should be guarded by GC concurrent collector, so we are using this ConcurrentJSLock
+ to guard them against concurrent collector too.
+ The problem is that ENABLE(CONCURRENT_JS) relies on ENABLE(DFG_JIT). If we configure JSC with the options like,
+
+ ENABLE_DFG_JIT 0
+ ENABLE_FTL_JIT 0
+
+ Then, the built JSC becomes
+
+ ENABLE_CONCURRENT_JS 0
+ But, Concurrent GC is enabled.
+
+ This is wrong due to several reasons.
+
+ 1. Baseline JIT can produce JIT related data structures that are traced by concurrent collector. In the above options,
+ these data structures are not guarded by lock.
+ 2. Baseline JIT also has concurrent JIT compiler. But ENABLE_CONCURRENT_JS does not reflect this.
+
+ In this patch, we fix two things.
+
+ 1. We should make ConcurrentJSLock always Lock. In 64bit environment we are supporting actively (including watchOS ARM64_32),
+ we are enabling ENABLE(JIT) regardless of we are actually using JIT. So, anyway, this is already a Lock. Flipping these
+ bits does not matter in 32bit architectures since they do not have concurrent compilers anyway. This makes things simpler:
+ it is always a Lock. And concurrent collector can use it.
+ 2. We should make `ENABLE(CONCURRENT_JS)` ON when `ENABLE(JIT)` is true, to reflect the fact that Baseline JIT has concurrent compiler.
+
+ * runtime/ConcurrentJSLock.h:
+ (JSC::ConcurrentJSLocker::ConcurrentJSLocker):
+
2019-09-18 Saam Barati <sbarati@apple.com>

Phantom insertion phase may disagree with arguments forwarding about live ranges
--- target-org/webkit/Source/JavaScriptCore/runtime/ConcurrentJSLock.h 2018-12-19 20:41:11.000000000 -0800
+++ target/webkit/Source/JavaScriptCore/runtime/ConcurrentJSLock.h 2019-10-21 11:43:39.000000000 -0700
@@ -32,13 +32,8 @@

namespace JSC {

-#if ENABLE(CONCURRENT_JS)
-typedef Lock ConcurrentJSLock;
-typedef LockHolder ConcurrentJSLockerImpl;
-#else
-typedef NoLock ConcurrentJSLock;
-typedef NoLockLocker ConcurrentJSLockerImpl;
-#endif
+using ConcurrentJSLock = Lock;
+using ConcurrentJSLockerImpl = LockHolder;

static_assert(sizeof(ConcurrentJSLock) == 1, "Regardless of status of concurrent JS flag, size of ConurrentJSLock is always one byte.");

@@ -103,7 +98,7 @@
public:
ConcurrentJSLocker(ConcurrentJSLock& lockable)
: ConcurrentJSLockerBase(lockable)
-#if ENABLE(CONCURRENT_JS) && !defined(NDEBUG)
+#if !defined(NDEBUG)
, m_disallowGC(std::in_place)
#endif
{
@@ -111,7 +106,7 @@

ConcurrentJSLocker(ConcurrentJSLock* lockable)
: ConcurrentJSLockerBase(lockable)
-#if ENABLE(CONCURRENT_JS) && !defined(NDEBUG)
+#if !defined(NDEBUG)
, m_disallowGC(std::in_place)
#endif
{
@@ -119,7 +114,7 @@

ConcurrentJSLocker(NoLockingNecessaryTag)
: ConcurrentJSLockerBase(NoLockingNecessary)
-#if ENABLE(CONCURRENT_JS) && !defined(NDEBUG)
+#if !defined(NDEBUG)
, m_disallowGC(WTF::nullopt)
#endif
{
@@ -127,7 +122,7 @@

ConcurrentJSLocker(int) = delete;

-#if ENABLE(CONCURRENT_JS) && !defined(NDEBUG)
+#if !defined(NDEBUG)
private:
Optional<DisallowGC> m_disallowGC;
#endif
--- target-org/webkit/Source/WTF/ChangeLog 2019-09-23 18:40:37.000000000 +0800
+++ target/webkit/Source/WTF/ChangeLog 2019-12-18 08:35:53.000000000 +0800
@@ -1,3 +1,15 @@
+2019-10-18 Yusuke Suzuki <ysuzuki@apple.com>
+
+ [JSC] Make ConcurrentJSLock Lock even if ENABLE_CONCURRENT_JS=OFF
+ https://bugs.webkit.org/show_bug.cgi?id=202892
+
+ Reviewed by Mark Lam.
+
+ BaselineJIT also has concurrent compiler. ENABLE(CONCURRENT_JS) should not rely on ENABLE(DFG_JIT).
+ It should rely on ENABLE(JIT) instead.
+
+ * wtf/Platform.h:
+
2019-09-20 Libor Bukata <libor.bukata@oracle.com>

UI process crash when using callOnMainThread() after the main thread dispatcher has been destroyed
--- target-org/webkit/Source/WTF/wtf/Platform.h 2019-08-23 14:21:51.000000000 -0700
+++ target/webkit/Source/WTF/wtf/Platform.h 2019-10-21 11:44:30.000000000 -0700
@@ -840,7 +840,7 @@
values get stored to atomically. This is trivially true on 64-bit platforms,
but not true at all on 32-bit platforms where values are composed of two
separate sub-values. */
-#if ENABLE(DFG_JIT) && USE(JSVALUE64)
+#if ENABLE(JIT) && USE(JSVALUE64)
#define ENABLE_CONCURRENT_JS 1
#endif
4 changes: 4 additions & 0 deletions scripts/patch.sh
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ JSC_PATCHSET=(

# Improve heap GC mechanism like iOS
"jsc_heap_gc_like_ios.patch"

# GC concurrent issue potential fix
# https://trac.webkit.org/changeset/251307/webkit
"jsc_fix_concurrent_gc_issue.patch"
)

if [[ "$I18N" = false ]]
Expand Down