Bug 118067 – [GTK] Use the GCActivityCallback

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Sergio Villar Senin]

Looks like it will help by improving the efficiency of use of the JSC heap.

See Qt's implementation in bug 103996 & bug 103998
EFL implementation in bug 95923

[Peng Xinchao]

Created attachment 207172 [details]
implement gtk GCActivityCallback

Additional GCs can be triggered by platform timer. It has sort of compaction effect not to make JSC heap grow fast so that memory usage becomes lower than usual

[Peng Xinchao]

Created attachment 207173 [details]
implement gtk GCActivityCallback

Additional GCs can be triggered by platform timer. It has sort of compaction effect not to make JSC heap grow fast so that memory usage becomes lower than usual

[Peng Xinchao]

Created attachment 207174 [details]
add changLog

[Peng Xinchao]

Created attachment 207175 [details]
Path

[Peng Xinchao]

Created attachment 207176 [details]
Patch

[Zan Dobersek]

The patch looks about right after a quick glance at it, but is still missing the changelog and the r? flag so someone can actually do a proper review. There's also some styling issues present.

It also needs to apply to the tree so the EWSs can process it.

[Sergio Villar Senin]

Comment on attachment 207176 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207176&action=review

Apart from the inline comments, remember that you have to add a proper ChangeLog with the description of the changes (maybe this helps http://trac.webkit.org/wiki/CodeReview)

> runtime/GCActivityCallback.h:72
> +    }

You don't need this if you aren't doing anything different from the #else following this code.

> runtime/GCActivityCallback.h:117
> +#endif

At least cancelTimer(), scheduleTimer() and m_delay are shared with the other ports. There should be a better way to write this.

> runtime/GCActivityCallback.cpp:44
> +#include <gdk/gdk.h>

Why gdk?

> runtime/GCActivityCallback.cpp:87
> +}

Isn't this exactly the same as Qt? Couldn't we do "#elif PLATFORM(QT) || PLATFORM(GTK)" ?

> runtime/GCActivityCallback.cpp:168
> +    

Check the indentation and remove the extra lines at the end of the function.

> runtime/GCActivityCallback.cpp:174
> +	stop();

Check indentation. Also the implementation is exactly the same as EFL, we might think about sharing it.

> runtime/GCActivityCallback.cpp:179
> +

Extra line.

> runtime/GCActivityCallback.cpp:181
> +		return;

Indentation.

> runtime/GCActivityCallback.cpp:202
> +		   return;

Indentation. Also any reason why we could not share EFL's code?. Actually, I don't know exactly about the details but looks like the EFL code might be shared among ports (not sure about the ASSERT though)

> heap/HeapTimer.cpp:44
> +#include <gdk/gdk.h>

Again why gdk?

[Peng Xinchao]

Created attachment 208390 [details]
accept these suggestiones of Sergio Villar Senin .

accept these suggestiones of Sergio Villar Senin .

[Peng Xinchao]

Created attachment 208391 [details]
accept these suggestiones of Sergio Villar Senin .

[Peng Xinchao]

(In reply to comment #7)
> (From update of attachment 207176 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207176&action=review
> 
> Apart from the inline comments, remember that you have to add a proper ChangeLog with the description of the changes (maybe this helps http://trac.webkit.org/wiki/CodeReview)
> 
> > runtime/GCActivityCallback.h:72
> > +    }
> 
> You don't need this if you aren't doing anything different from the #else following this code.
> 
> > runtime/GCActivityCallback.h:117
> > +#endif
> 
> At least cancelTimer(), scheduleTimer() and m_delay are shared with the other ports. There should be a better way to write this.
> 
> > runtime/GCActivityCallback.cpp:44
> > +#include <gdk/gdk.h>
> 
> Why gdk?
> 
> > runtime/GCActivityCallback.cpp:87
> > +}
> 
> Isn't this exactly the same as Qt? Couldn't we do "#elif PLATFORM(QT) || PLATFORM(GTK)" ?
> 
> > runtime/GCActivityCallback.cpp:168
> > +    
> 
> Check the indentation and remove the extra lines at the end of the function.
> 
> > runtime/GCActivityCallback.cpp:174
> > +	stop();
> 
> Check indentation. Also the implementation is exactly the same as EFL, we might think about sharing it.
> 
> > runtime/GCActivityCallback.cpp:179
> > +
> 
> Extra line.
> 
> > runtime/GCActivityCallback.cpp:181
> > +		return;
> 
> Indentation.
> 
> > runtime/GCActivityCallback.cpp:202
> > +		   return;
> 
> Indentation. Also any reason why we could not share EFL's code?. Actually, I don't know exactly about the details but looks like the EFL code might be shared among ports (not sure about the ASSERT though)
> 
> > heap/HeapTimer.cpp:44
> > +#include <gdk/gdk.h>
> 
> Again why gdk?

I motify codes ,but Why  my new patch  do not display normal?

[Peng Xinchao]

Created attachment 211397 [details]
implement gtk GCActivityCallback

[Peng Xinchao]

In pressure test, i happened the crash :
 i feel odd ,please check .

(gdb) bt full
#0  0xb6158870 in JSC::ExecutableBase::clearCode() () from ../../../SymbolFiles/mtd_exe/Webkit/libjavascriptcoregtk-3.0.so.0
No symbol table info available.
#1  0xb6159440 in JSC::FunctionExecutable::clearCode() () from ../../../SymbolFiles/mtd_exe/Webkit/libjavascriptcoregtk-3.0.so.0
No symbol table info available.
#2  0xb6159468 in JSC::FunctionExecutable::clearCodeIfNotCompiling() () from ../../../SymbolFiles/mtd_exe/Webkit/libjavascriptcoregtk-3.0.so.0
No symbol table info available.
#3  0xb603ebf4 in JSC::Heap::deleteAllCompiledCode() () from ../../../SymbolFiles/mtd_exe/Webkit/libjavascriptcoregtk-3.0.so.0
No symbol table info available.
#4  0xb603f59c in JSC::Heap::collect(JSC::Heap::SweepToggle) () from ../../../SymbolFiles/mtd_exe/Webkit/libjavascriptcoregtk-3.0.so.0
No symbol table info available.
#5  0xb61623e0 in JSC::DefaultGCActivityCallback::doWork() () from ../../../SymbolFiles/mtd_exe/Webkit/libjavascriptcoregtk-3.0.so.0
No symbol table info available.
#6  0xa6f39a40 in g_timeout_dispatch () from ../../../SymbolFiles/mtd_cmmlib/Runtime/lib/libglib-2.0.so.0
No symbol table info available.
#7  0xa6f38cd4 in g_main_context_dispatch () from ../../../SymbolFiles/mtd_cmmlib/Runtime/lib/libglib-2.0.so.0
No symbol table info available.
#8  0xa6f3904c in g_main_context_iterate () from ../../../SymbolFiles/mtd_cmmlib/Runtime/lib/libglib-2.0.so.0
No symbol table info available.
#9  0xa6f39510 in g_main_loop_run () from ../../../SymbolFiles/mtd_cmmlib/Runtime/lib/libglib-2.0.so.0
No symbol table info available.
#10 0xb5878c94 in WebCore::RunLoop::run() () from ../../../SymbolFiles/mtd_exe/Webkit/libwebkit2gtk-3.0.so.0
No symbol table info available.
#11 0xb4292b0c in WebProcessMainGtk () from ../../../SymbolFiles/mtd_exe/Webkit/libwebkit2gtk-3.0.so.0
No symbol table info available.
#12 0x00008cc4 in main ()

[Peng Xinchao]

Created attachment 219037 [details]
Patch

[Peng Xinchao]

Created attachment 219038 [details]
Patch

[EFL EWS Bot]

Comment on attachment 219038 [details]
Patch

Attachment 219038 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/47618165

[Build Bot]

Comment on attachment 219038 [details]
Patch

Attachment 219038 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/48118141

[Build Bot]

Comment on attachment 219038 [details]
Patch

Attachment 219038 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/48148157

[EFL EWS Bot]

Comment on attachment 219038 [details]
Patch

Attachment 219038 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/47318184

[Build Bot]

Comment on attachment 219038 [details]
Patch

Attachment 219038 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/45068194

[Build Bot]

Comment on attachment 219038 [details]
Patch

Attachment 219038 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/48088158

[Sergio Villar Senin]

(In reply to comment #14)
> Created an attachment (id=219038) [details]
> Patch

../../Source/JavaScriptCore/runtime/GCActivityCallback.cpp:51:33: error: missing ')' in expression
 #if USE(CF) || PLATFORM(EFL) || (PLATFORM(GTK)

You have to remove that extra parenthesis

[kov's GTK+ EWS bot]

Comment on attachment 219038 [details]
Patch

Attachment 219038 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/48228016

[Peng Xinchao]

Created attachment 219511 [details]
Patch

[Michael Catanzaro]

Comment on attachment 219511 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=219511&action=review

Looks like we wound up duplicating work due to not landing this patch. Another implementation of this landed in r192773. Oh well....

> Source/JavaScriptCore/heap/HeapTimer.cpp:173
> +    ASSERT(m_mainLoop);

I don't think these assertions are useful; they're never going to fail. (Note that GLib crashes if memory allocation fails.)

[Michael Catanzaro]

*** This bug has been marked as a duplicate of bug 151391 ***