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
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
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
Created attachment 207174 [details] add changLog
Created attachment 207175 [details] Path
Created attachment 207176 [details] Patch
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.
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?
Created attachment 208390 [details] accept these suggestiones of Sergio Villar Senin . accept these suggestiones of Sergio Villar Senin .
Created attachment 208391 [details] accept these suggestiones of Sergio Villar Senin .
(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?
Created attachment 211397 [details] implement gtk GCActivityCallback
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 ()
Created attachment 219037 [details] Patch
Created attachment 219038 [details] Patch
Comment on attachment 219038 [details] Patch Attachment 219038 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/47618165
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
Comment on attachment 219038 [details] Patch Attachment 219038 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/48148157
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
Comment on attachment 219038 [details] Patch Attachment 219038 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/45068194
Comment on attachment 219038 [details] Patch Attachment 219038 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/48088158
(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
Comment on attachment 219038 [details] Patch Attachment 219038 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/48228016
Created attachment 219511 [details] Patch
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.)
*** This bug has been marked as a duplicate of bug 151391 ***