Bug 95923 – [EFL] Implement GCActivityCallback

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Hojong Han]

to add a GCActivityCallbackEfl.cpp, 
and implement GCActivityCallback with platform timer

[Hojong Han]

Created attachment 162393 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 162393 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Implement GCActivityCallback and HeapTimer for EFL port.

It would be good if you could explain what you need these for.

[Gyuyoung Kim]

Comment on attachment 162393 [details]
Patch

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

> Source/JavaScriptCore/PlatformEfl.cmake:2
> +    runtime/GCActivityCallback.cpp

In this file case, I think we're able to remove this file in CMakeLists.txt. Then, we can add a port specific file to PlatformXXX.cmake by port itself. So, I'm going to file a bug for this.

> Source/JavaScriptCore/PlatformEfl.cmake:10
> +    runtime/GCActivityCallbackEfl.cpp

Wrong alphabetical order.

> Source/JavaScriptCore/PlatformEfl.cmake:19
> +    ${ECORE_X_INCLUDE_DIRS}

ditto.

> Source/JavaScriptCore/heap/HeapTimer.cpp:147
> +        ecore_timer_del(m_timer);

A new line ?

> Source/JavaScriptCore/heap/HeapTimer.h:77
> +    Ecore_Timer* m_timer;

Can't you use OwnPtr here ? If you can use OwnPtr for this, you'd be able to remove most of the checks for m_timer as well as calls to ecore_timer_del() in this patch.

See also, RunLoopEfl.h / cpp.

[Hojong Han]

Created attachment 162655 [details]
Patch

[Hojong Han]

(In reply to comment #2)
> (From update of attachment 162393 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162393&action=review
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        Implement GCActivityCallback and HeapTimer for EFL port.
> 
> It would be good if you could explain what you need these for.
These are for efficient use of JSC heap.
JSC heap grows rapidly at some web sites and it causes fragmentation a lot in marked blocks so that we put the codes calling collect without sweep according to the allocated size in order to make use of available spaces in marked blocks better.

[Simon Hausmann]

I see an issue here that is not EFL specific:

The only thing that is not cross-platform in an implementation of HeapTimer and GCActivityCallback is the timer. The rest is copy & paste across the ports. Wouldn't it be nicer to have the simple runloop timer functionality needed here abstracted away in WTF and make these two JSC classes cross-platform?

Kenneth, what do you think? Doesn't this smell like an opportunity for code sharing? :)

[Kenneth Rohde Christiansen]

(In reply to comment #6)
> I see an issue here that is not EFL specific:
> 
> The only thing that is not cross-platform in an implementation of HeapTimer and GCActivityCallback is the timer. The rest is copy & paste across the ports. Wouldn't it be nicer to have the simple runloop timer functionality needed here abstracted away in WTF and make these two JSC classes cross-platform?
> 
> Kenneth, what do you think? Doesn't this smell like an opportunity for code sharing? :)

I think that sounds like a good idea.

[Gyuyoung Kim]

(In reply to comment #7)
> (In reply to comment #6)
> > I see an issue here that is not EFL specific:
> > 
> > The only thing that is not cross-platform in an implementation of HeapTimer and GCActivityCallback is the timer. The rest is copy & paste across the ports. Wouldn't it be nicer to have the simple runloop timer functionality needed here abstracted away in WTF and make these two JSC classes cross-platform?
> > 
> > Kenneth, what do you think? Doesn't this smell like an opportunity for code sharing? :)
> 
> I think that sounds like a good idea.

I think so. BTW, do you think this patch supports it as well? Isn’t it better to resolve that in a new bug after finishing this bug ? :-)

[Hojong Han]

Geoffrey, 
Could you take a look into this patch?
Give me any feedback in terms of JSC heap.

[Gyuyoung Kim]

Comment on attachment 162393 [details]
Patch

Hojong, I wonder if EFL port still needs to have this patch.

[Hojong Han]

(In reply to comment #10)
> (From update of attachment 162393 [details])
> Hojong, I wonder if EFL port still needs to have this patch.

Yes, I think it needs this.

[Gyuyoung Kim]

Comment on attachment 162655 [details]
Patch

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

> Source/JavaScriptCore/PlatformEfl.cmake:18
> +    ${ECORE_X_INCLUDE_DIRS}

Why do you incluce ECORE_X instead of ECORE ? There is no ecore_x usage in this patch.

[Chris Dumez]

Comment on attachment 162655 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Implement GCActivityCallback and HeapTimer for EFL port.

Please explain in the changelog why those are needed (i.e. the benefit of implementing this).

[Hojong Han]

Created attachment 177172 [details]
Patch

[Hojong Han]

(In reply to comment #12)
> (From update of attachment 162655 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162655&action=review
> 
> > Source/JavaScriptCore/PlatformEfl.cmake:18
> > +    ${ECORE_X_INCLUDE_DIRS}
> 
> Why do you incluce ECORE_X instead of ECORE ? There is no ecore_x usage in this patch.
I changed it to ECORE and added ENIA which should be included with ECORE.

[Gyuyoung Kim]

Comment on attachment 177172 [details]
Patch

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

> Source/JavaScriptCore/PlatformEfl.cmake:2
> +    runtime/GCActivityCallback.cpp

As Simon said, I also think this file is not cross-platform. At least, I think we shouldn't remove existing file for port implementation as here. Generally, each port has its own implementation file for similar case. For example, in this case, I think we should have GCActivityCallbackEfl.h/.cpp for now at least. I think you need to file a new bug for this issue regardless of which method is better(Simon's suggestion or support for each port implementation(CF, blackberry, EFL port and so on). Then, I think you can add this patch for EFL port.

[Simon Hausmann]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > I see an issue here that is not EFL specific:
> > > 
> > > The only thing that is not cross-platform in an implementation of HeapTimer and GCActivityCallback is the timer. The rest is copy & paste across the ports. Wouldn't it be nicer to have the simple runloop timer functionality needed here abstracted away in WTF and make these two JSC classes cross-platform?
> > > 
> > > Kenneth, what do you think? Doesn't this smell like an opportunity for code sharing? :)
> > 
> > I think that sounds like a good idea.
> 
> I think so. BTW, do you think this patch supports it as well? Isn’t it better to resolve that in a new bug after finishing this bug ? :-)

Given that this doesn't seem to be urgent (the patch has been hanging around here for a good while now), why not do it all in one shot?

[Allan Sandfeld Jensen]

Btw, see my patch in bug #103996 https://bugs.webkit.org/attachment.cgi?id=181678&action=prettypatch

I have tried to split HeapTimer.cpp into platform specific files. EFL would need your own file under that system.

[Gyuyoung Kim]

(In reply to comment #18)
> Btw, see my patch in bug #103996 https://bugs.webkit.org/attachment.cgi?id=181678&action=prettypatch
> 
> I have tried to split HeapTimer.cpp into platform specific files. EFL would need your own file under that system.

I'm considering to split HeapTimer.cpp into each port file on Bug 96054. If you land it, I think this patch also should follow it. BTW, how do you think to split GCACtivityCallback.cpp as HeapTimer.cpp ?

[Hojong Han]

Created attachment 202494 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 202494 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        [EFL] Implement GCActivityCallback

It looks you need to change bug title according to new patch.

> Source/JavaScriptCore/PlatformEfl.cmake:2
> +    ${ECORE_X_INCLUDE_DIRS}

It looks you don't use ecore_x. So, ${ECORE_INCLUDE_DIRS} is enough.

> Source/JavaScriptCore/runtime/GCActivityCallback.cpp:-105
> -

Do not modify code unrelated to this patch.

> Source/JavaScriptCore/runtime/GCActivityCallback.cpp:124
> +    stop();

Why do you need to call stop() here ?

[Hojong Han]

(In reply to comment #21)
> (From update of attachment 202494 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202494&action=review
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        [EFL] Implement GCActivityCallback
> 
> It looks you need to change bug title according to new patch.

Title changed to match with the patch.

> > Source/JavaScriptCore/PlatformEfl.cmake:2
> > +    ${ECORE_X_INCLUDE_DIRS}
> 
> It looks you don't use ecore_x. So, ${ECORE_INCLUDE_DIRS} is enough.

yes, ECORE_INCLUDE_DIRS is enough. I'll change it.

> > Source/JavaScriptCore/runtime/GCActivityCallback.cpp:-105
> > -
> 
> Do not modify code unrelated to this patch.

It's just kind of improving readability, but no biggie whether keep it as it was.

> > Source/JavaScriptCore/runtime/GCActivityCallback.cpp:124
> > +    stop();
> 
> Why do you need to call stop() here ?

Doesn't it need to stop the previous before setting new one ?

[Hojong Han]

Created attachment 202500 [details]
Patch

[Chris Dumez]

Comment on attachment 202500 [details]
Patch

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

> Source/JavaScriptCore/heap/HeapTimer.cpp:188
> +    return ECORE_CALLBACK_CANCEL;

You need to set heapTimer->m_timer to 0 before returning. When you return ECORE_CALLBACK_CANCEL, the handle will become invalid and ecore_timer_del(m_timer) will fail in the destructor.

> Source/JavaScriptCore/heap/HeapTimer.h:42
> +#include <Ecore.h>

A forward declaration of Ecore_Timer would suffice here. And this include can be moved to the cpp.

> Source/JavaScriptCore/heap/HeapTimer.h:87
> +    void stop()

Can you put the method body in the cpp file instead?

> Source/JavaScriptCore/heap/HeapTimer.h:89
> +        if (m_timer) {

We usually do the opposite in WebKit, return early.
If (!m_timer)
    return;

> Source/JavaScriptCore/runtime/GCActivityCallback.cpp:127
> +    m_timer = ecore_timer_add(newDelay, reinterpret_cast<Ecore_Task_Cb>(HeapTimer::timerEvent), this);

I would add a ASSERT(!m_timer); right before this line to make sure we never leak.

[Hojong Han]

Comment on attachment 202500 [details]
Patch

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

>> Source/JavaScriptCore/heap/HeapTimer.cpp:188
>> +    return ECORE_CALLBACK_CANCEL;
> 
> You need to set heapTimer->m_timer to 0 before returning. When you return ECORE_CALLBACK_CANCEL, the handle will become invalid and ecore_timer_del(m_timer) will fail in the destructor.

I'll apply your review to the patch.

>> Source/JavaScriptCore/heap/HeapTimer.h:42
>> +#include <Ecore.h>
> 
> A forward declaration of Ecore_Timer would suffice here. And this include can be moved to the cpp.

Do you mean that it'd better move "#include <Ecore.h>" to the cpp?

>> Source/JavaScriptCore/heap/HeapTimer.h:87
>> +    void stop()
> 
> Can you put the method body in the cpp file instead?

This'll be moved as your comment.

>> Source/JavaScriptCore/heap/HeapTimer.h:89
>> +        if (m_timer) {
> 
> We usually do the opposite in WebKit, return early.
> If (!m_timer)
>     return;

This'll be moved as your comment.

>> Source/JavaScriptCore/runtime/GCActivityCallback.cpp:127
>> +    m_timer = ecore_timer_add(newDelay, reinterpret_cast<Ecore_Task_Cb>(HeapTimer::timerEvent), this);
> 
> I would add a ASSERT(!m_timer); right before this line to make sure we never leak.

I'll fix to set m_timer as 0 in stop().

[Chris Dumez]

Comment on attachment 202500 [details]
Patch

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

>>> Source/JavaScriptCore/heap/HeapTimer.h:42
>>> +#include <Ecore.h>
>> 
>> A forward declaration of Ecore_Timer would suffice here. And this include can be moved to the cpp.
> 
> Do you mean that it'd better move "#include <Ecore.h>" to the cpp?

Yes, and have a forward declaration here:
typedef struct _Ecore_Timer Ecore_Timer;

>>> Source/JavaScriptCore/runtime/GCActivityCallback.cpp:127
>>> +    m_timer = ecore_timer_add(newDelay, reinterpret_cast<Ecore_Task_Cb>(HeapTimer::timerEvent), this);
>> 
>> I would add a ASSERT(!m_timer); right before this line to make sure we never leak.
> 
> I'll fix to set m_timer as 0 in stop().

You already set m_timer to 0 in stop. All I ask for in an extra assert to make sure someone does not remove the stop() later on :)

[Gyuyoung Kim]

Comment on attachment 202500 [details]
Patch

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

> Source/JavaScriptCore/heap/HeapTimer.cpp:174
> +    ecore_init();

Why do you need to call ecore_init() again though it is already called during the webprocess initialization ?

> Source/JavaScriptCore/heap/HeapTimer.cpp:180
> +    ecore_shutdown();

ditto.

[Hojong Han]

Comment on attachment 202500 [details]
Patch

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

>>>> Source/JavaScriptCore/heap/HeapTimer.h:42
>>>> +#include <Ecore.h>
>>> 
>>> A forward declaration of Ecore_Timer would suffice here. And this include can be moved to the cpp.
>> 
>> Do you mean that it'd better move "#include <Ecore.h>" to the cpp?
> 
> Yes, and have a forward declaration here:
> typedef struct _Ecore_Timer Ecore_Timer;

HeapTimer.h is included in GCActivityCallback.h and there's Ecore_Task_Cb in GCActivityCallback.cpp.
What do you think keeping it in HeapTimer.h?

>>>> Source/JavaScriptCore/runtime/GCActivityCallback.cpp:127
>>>> +    m_timer = ecore_timer_add(newDelay, reinterpret_cast<Ecore_Task_Cb>(HeapTimer::timerEvent), this);
>>> 
>>> I would add a ASSERT(!m_timer); right before this line to make sure we never leak.
>> 
>> I'll fix to set m_timer as 0 in stop().
> 
> You already set m_timer to 0 in stop. All I ask for in an extra assert to make sure someone does not remove the stop() later on :)

yes, it's good to apply for the case as you mentioned.

[Chris Dumez]

Comment on attachment 202500 [details]
Patch

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

>>>>> Source/JavaScriptCore/heap/HeapTimer.h:42
>>>>> +#include <Ecore.h>
>>>> 
>>>> A forward declaration of Ecore_Timer would suffice here. And this include can be moved to the cpp.
>>> 
>>> Do you mean that it'd better move "#include <Ecore.h>" to the cpp?
>> 
>> Yes, and have a forward declaration here:
>> typedef struct _Ecore_Timer Ecore_Timer;
> 
> HeapTimer.h is included in GCActivityCallback.h and there's Ecore_Task_Cb in GCActivityCallback.cpp.
> What do you think keeping it in HeapTimer.h?

No, please add the proper include to GCActivityCallback.cpp then.

[Hojong Han]

Comment on attachment 202500 [details]
Patch

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

>> Source/JavaScriptCore/heap/HeapTimer.cpp:174
>> +    ecore_init();
> 
> Why do you need to call ecore_init() again though it is already called during the webprocess initialization ?

There's a binary to run in shell. That binary runs without ecore_init but ecore_timer is in GCActivityCallback.

[Hojong Han]

Created attachment 202503 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #30)
> (From update of attachment 202500 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202500&action=review
> 
> >> Source/JavaScriptCore/heap/HeapTimer.cpp:174
> >> +    ecore_init();
> > 
> > Why do you need to call ecore_init() again though it is already called during the webprocess initialization ?
> 
> There's a binary to run in shell. That binary runs without ecore_init but ecore_timer is in GCActivityCallback.

If so, how about adding it to main() of Jsc.cpp ? I don't agree to call ecore_init() / ecore_shutdown() again here. I heard that there may be problems inside EFL when we call it twice.

[Chris Dumez]

Comment on attachment 202503 [details]
Patch

Similarly to the BlackBerry implementation, your HeapTimer implementation will only work for the main thread. I would make that obvious with a comment and a check in the constructor, as BlackBerry is doing. Please make sure to test your patch in debug mode to make sure you're not introducing crashes.

[Chris Dumez]

(In reply to comment #32)
> (In reply to comment #30)
> > (From update of attachment 202500 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202500&action=review
> > 
> > >> Source/JavaScriptCore/heap/HeapTimer.cpp:174
> > >> +    ecore_init();
> > > 
> > > Why do you need to call ecore_init() again though it is already called during the webprocess initialization ?
> > 
> > There's a binary to run in shell. That binary runs without ecore_init but ecore_timer is in GCActivityCallback.
> 
> If so, how about adding it to main() of Jsc.cpp ? I don't agree to call ecore_init() / ecore_shutdown() again here. I heard that there may be problems inside EFL when we call it twice.

if you ecore_init() more than once, it should merely increase a ref count and not do any processing.

[Hojong Han]

Comment on attachment 202500 [details]
Patch

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

>>>> Source/JavaScriptCore/heap/HeapTimer.cpp:174
>>>> +    ecore_init();
>>> 
>>> Why do you need to call ecore_init() again though it is already called during the webprocess initialization ?
>> 
>> There's a binary to run in shell. That binary runs without ecore_init but ecore_timer is in GCActivityCallback.
> 
> If so, how about adding it to main() of Jsc.cpp ? I don't agree to call ecore_init() / ecore_shutdown() again here. I heard that there may be problems inside EFL when we call it twice.

Good to move those in Jsc.cpp even if there isn't any specific problem when calling it more than twice as I know of :)

>> Source/JavaScriptCore/heap/HeapTimer.cpp:180
>> +    ecore_shutdown();
> 
> ditto.

ditto.

[Hojong Han]

Created attachment 202505 [details]
Patch

[Chris Dumez]

Comment on attachment 202505 [details]
Patch

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

> Source/JavaScriptCore/heap/HeapTimer.cpp:175
> +{

Please see my earlier comment about adding a thread check here.

> Source/JavaScriptCore/jsc.cpp:805
> +    ShellEnvironment env;

Why add this to jscmain instead of main() like other ports?

[Gyuyoung Kim]

Comment on attachment 202505 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Implements the activity triggered garbage collector.

It looks this description is insufficient for this patch. Please add more detailed description.

[Hojong Han]

Comment on attachment 202505 [details]
Patch

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

>> Source/JavaScriptCore/heap/HeapTimer.cpp:175
>> +{
> 
> Please see my earlier comment about adding a thread check here.

HeapTimer can be created in other than main thread, but it does not make m_timer run because other threads might not have an ecore loop.
In this vein, ecore_init() in constructor and ecore_shutdown() in destructor are necessary.

>> Source/JavaScriptCore/jsc.cpp:805
>> +    ShellEnvironment env;
> 
> Why add this to jscmain instead of main() like other ports?

I'll remove this if my comment upper is acceptable

[Hojong Han]

Created attachment 202524 [details]
Patch

GCActivityCallback works only in main thread

[WebKit Commit Bot]

Attachment 202524 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/PlatformEfl.cmake', u'Source/JavaScriptCore/heap/HeapTimer.cpp', u'Source/JavaScriptCore/heap/HeapTimer.h', u'Source/JavaScriptCore/jsc.cpp', u'Source/JavaScriptCore/runtime/GCActivityCallback.cpp', u'Source/JavaScriptCore/runtime/GCActivityCallback.h']" exit_code: 1
Source/JavaScriptCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Hojong Han]

Created attachment 202526 [details]
Patch

style error fixed

[Gyuyoung Kim]

Comment on attachment 202526 [details]
Patch

Looks good to me for EFL port. However, I'd like to know how does JS reviewer think about this patch. Geoffrey, what do you think about this ?

[Geoffrey Garen]

Comment on attachment 202526 [details]
Patch

Please use #endif followed by #if instead of #elif. For platform #ifdefs, that makes it easier to edit code that's not related to your particular port.

[Hojong Han]

(In reply to comment #44)
> (From update of attachment 202526 [details])
> Please use #endif followed by #if instead of #elif. For platform #ifdefs, that makes it easier to edit code that's not related to your particular port.
Thanks for review, Geoffrey.
But one thing not clear is which #elifs you meant. #elifs for QT and BlackBerry had been used so that I complied with those.
Do you want every #elif, on this patch, to follow your guide or just several parts such as including header file?

[Geoffrey Garen]

Ah, I didn't realize. I guess we can save the #if/#elif cleanup for a follow-up patch.

[WebKit Commit Bot]

Comment on attachment 202526 [details]
Patch

Clearing flags on attachment: 202526

Committed r151149: <http://trac.webkit.org/changeset/151149>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.