Skip to content

Commit 57a2aa0

Browse files
tosbahaKudo
tosbaha
authored and
Kudo
committed
GC concurrent issue potential fix
1 parent afb6f53 commit 57a2aa0

File tree

2 files changed

+129
-0
lines changed

2 files changed

+129
-0
lines changed

‎patches/jsc_fix_concurrent_gc_issue.patch

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
--- target-org/webkit/Source/JavaScriptCore/ChangeLog 2019-09-23 18:14:45.000000000 +0800
2+
+++ target/webkit/Source/JavaScriptCore/ChangeLog 2019-12-18 08:36:29.000000000 +0800
3+
@@ -1,3 +1,40 @@
4+
+2019-10-18 Yusuke Suzuki <[email protected]>
5+
+
6+
+ [JSC] Make ConcurrentJSLock Lock even if ENABLE_CONCURRENT_JS=OFF
7+
+ https://bugs.webkit.org/show_bug.cgi?id=202892
8+
+
9+
+ Reviewed by Mark Lam.
10+
+
11+
+ We are using ConcurrentJSLock to guard data structure against concurrent compilers.
12+
+ But these data structures should be guarded by GC concurrent collector, so we are using this ConcurrentJSLock
13+
+ to guard them against concurrent collector too.
14+
+ The problem is that ENABLE(CONCURRENT_JS) relies on ENABLE(DFG_JIT). If we configure JSC with the options like,
15+
+
16+
+ ENABLE_DFG_JIT 0
17+
+ ENABLE_FTL_JIT 0
18+
+
19+
+ Then, the built JSC becomes
20+
+
21+
+ ENABLE_CONCURRENT_JS 0
22+
+ But, Concurrent GC is enabled.
23+
+
24+
+ This is wrong due to several reasons.
25+
+
26+
+ 1. Baseline JIT can produce JIT related data structures that are traced by concurrent collector. In the above options,
27+
+ these data structures are not guarded by lock.
28+
+ 2. Baseline JIT also has concurrent JIT compiler. But ENABLE_CONCURRENT_JS does not reflect this.
29+
+
30+
+ In this patch, we fix two things.
31+
+
32+
+ 1. We should make ConcurrentJSLock always Lock. In 64bit environment we are supporting actively (including watchOS ARM64_32),
33+
+ we are enabling ENABLE(JIT) regardless of we are actually using JIT. So, anyway, this is already a Lock. Flipping these
34+
+ bits does not matter in 32bit architectures since they do not have concurrent compilers anyway. This makes things simpler:
35+
+ it is always a Lock. And concurrent collector can use it.
36+
+ 2. We should make `ENABLE(CONCURRENT_JS)` ON when `ENABLE(JIT)` is true, to reflect the fact that Baseline JIT has concurrent compiler.
37+
+
38+
+ * runtime/ConcurrentJSLock.h:
39+
+ (JSC::ConcurrentJSLocker::ConcurrentJSLocker):
40+
+
41+
2019-09-18 Saam Barati <[email protected]>
42+
43+
Phantom insertion phase may disagree with arguments forwarding about live ranges
44+
--- target-org/webkit/Source/JavaScriptCore/runtime/ConcurrentJSLock.h 2018-12-19 20:41:11.000000000 -0800
45+
+++ target/webkit/Source/JavaScriptCore/runtime/ConcurrentJSLock.h 2019-10-21 11:43:39.000000000 -0700
46+
@@ -32,13 +32,8 @@
47+
48+
namespace JSC {
49+
50+
-#if ENABLE(CONCURRENT_JS)
51+
-typedef Lock ConcurrentJSLock;
52+
-typedef LockHolder ConcurrentJSLockerImpl;
53+
-#else
54+
-typedef NoLock ConcurrentJSLock;
55+
-typedef NoLockLocker ConcurrentJSLockerImpl;
56+
-#endif
57+
+using ConcurrentJSLock = Lock;
58+
+using ConcurrentJSLockerImpl = LockHolder;
59+
60+
static_assert(sizeof(ConcurrentJSLock) == 1, "Regardless of status of concurrent JS flag, size of ConurrentJSLock is always one byte.");
61+
62+
@@ -103,7 +98,7 @@
63+
public:
64+
ConcurrentJSLocker(ConcurrentJSLock& lockable)
65+
: ConcurrentJSLockerBase(lockable)
66+
-#if ENABLE(CONCURRENT_JS) && !defined(NDEBUG)
67+
+#if !defined(NDEBUG)
68+
, m_disallowGC(std::in_place)
69+
#endif
70+
{
71+
@@ -111,7 +106,7 @@
72+
73+
ConcurrentJSLocker(ConcurrentJSLock* lockable)
74+
: ConcurrentJSLockerBase(lockable)
75+
-#if ENABLE(CONCURRENT_JS) && !defined(NDEBUG)
76+
+#if !defined(NDEBUG)
77+
, m_disallowGC(std::in_place)
78+
#endif
79+
{
80+
@@ -119,7 +114,7 @@
81+
82+
ConcurrentJSLocker(NoLockingNecessaryTag)
83+
: ConcurrentJSLockerBase(NoLockingNecessary)
84+
-#if ENABLE(CONCURRENT_JS) && !defined(NDEBUG)
85+
+#if !defined(NDEBUG)
86+
, m_disallowGC(WTF::nullopt)
87+
#endif
88+
{
89+
@@ -127,7 +122,7 @@
90+
91+
ConcurrentJSLocker(int) = delete;
92+
93+
-#if ENABLE(CONCURRENT_JS) && !defined(NDEBUG)
94+
+#if !defined(NDEBUG)
95+
private:
96+
Optional<DisallowGC> m_disallowGC;
97+
#endif
98+
--- target-org/webkit/Source/WTF/ChangeLog 2019-09-23 18:40:37.000000000 +0800
99+
+++ target/webkit/Source/WTF/ChangeLog 2019-12-18 08:35:53.000000000 +0800
100+
@@ -1,3 +1,15 @@
101+
+2019-10-18 Yusuke Suzuki <[email protected]>
102+
+
103+
+ [JSC] Make ConcurrentJSLock Lock even if ENABLE_CONCURRENT_JS=OFF
104+
+ https://bugs.webkit.org/show_bug.cgi?id=202892
105+
+
106+
+ Reviewed by Mark Lam.
107+
+
108+
+ BaselineJIT also has concurrent compiler. ENABLE(CONCURRENT_JS) should not rely on ENABLE(DFG_JIT).
109+
+ It should rely on ENABLE(JIT) instead.
110+
+
111+
+ * wtf/Platform.h:
112+
+
113+
2019-09-20 Libor Bukata <[email protected]>
114+
115+
UI process crash when using callOnMainThread() after the main thread dispatcher has been destroyed
116+
--- target-org/webkit/Source/WTF/wtf/Platform.h 2019-08-23 14:21:51.000000000 -0700
117+
+++ target/webkit/Source/WTF/wtf/Platform.h 2019-10-21 11:44:30.000000000 -0700
118+
@@ -840,7 +840,7 @@
119+
values get stored to atomically. This is trivially true on 64-bit platforms,
120+
but not true at all on 32-bit platforms where values are composed of two
121+
separate sub-values. */
122+
-#if ENABLE(DFG_JIT) && USE(JSVALUE64)
123+
+#if ENABLE(JIT) && USE(JSVALUE64)
124+
#define ENABLE_CONCURRENT_JS 1
125+
#endif

‎scripts/patch.sh

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,10 @@ JSC_PATCHSET=(
6060

6161
# Improve heap GC mechanism like iOS
6262
"jsc_heap_gc_like_ios.patch"
63+
64+
# GC concurrent issue potential fix
65+
# https://trac.webkit.org/changeset/251307/webkit
66+
"jsc_fix_concurrent_gc_issue.patch"
6367
)
6468

6569
if [[ "$I18N" = false ]]

0 commit comments

Comments
 (0)